openstack/nova
Code Issues Proposed changes

[Trivial] docstrings, typos, minor refactoring

Browse Source 
As I was reading code trying to understand how PCI devices are handled,
I fixed some typos, added some docstrings, and did some minor
refactoring for readability/maintainability.

Change-Id: I7701c15207f4d386d46d8b0121bb44e12084f09b
This commit is contained in:
Eric Fried 2017-08-14 18:15:51 -05:00
parent 9c7d73195e
commit 4ec1668642
15 changed files with 131 additions and 106 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
nova/compute/api.py
View File

@ -1180,7 +1180,7 @@ class API(base.Base):
block_device_mapping=block_device_mapping,
tags=tags)
return (instances, reservation_id)
return instances, reservation_id
@staticmethod
def _cleanup_build_artifacts(instances, instances_to_build):

10
nova/compute/claims.py
View File

@ -196,7 +196,7 @@ class Claim(NopClaim):
if pci_requests.requests:
stats = self.tracker.pci_tracker.stats
if not stats.support_requests(pci_requests.requests):
return _('Claim pci failed.')
return _('Claim pci failed')
def _test_numa_topology(self, resources, limit):
host_topology = (resources.numa_topology
@ -219,12 +219,12 @@ class Claim(NopClaim):
if requested_topology and not instance_topology:
if pci_requests.requests:
return (_("Requested instance NUMA topology together with"
" requested PCI devices cannot fit the given"
" host NUMA topology"))
return (_("Requested instance NUMA topology together with "
"requested PCI devices cannot fit the given "
"host NUMA topology"))
else:
return (_("Requested instance NUMA topology cannot fit "
"the given host NUMA topology"))
"the given host NUMA topology"))
elif instance_topology:
self.claimed_numa_topology = instance_topology

4
nova/compute/manager.py
View File

@ -1109,8 +1109,8 @@ class ComputeManager(manager.Manager):
# if the configuration is wrong.
whitelist.Whitelist(CONF.pci.passthrough_whitelist)
# NOTE(sbauza): We want the compute node to hard fail if it can't be
# able to provide its resources to the placement API, or it would not
# NOTE(sbauza): We want the compute node to hard fail if it won't be
# able to provide its resources to the placement API, or it will not
# be able to be eligible as a destination.
if CONF.placement.os_region_name is None:
raise exception.PlacementNotConfigured()

2
nova/compute/resource_tracker.py
View File

@ -193,7 +193,7 @@ class ResourceTracker(object):
overhead = self.driver.estimate_instance_overhead(instance)
LOG.debug("Memory overhead for %(flavor)d MB instance; %(overhead)d "
"MB", {'flavor': instance.flavor.memory_mb,
'overhead': overhead['memory_mb']})
'overhead': overhead['memory_mb']})
LOG.debug("Disk overhead for %(flavor)d GB instance; %(overhead)d "
"GB", {'flavor': instance.flavor.root_gb,
'overhead': overhead.get('disk_gb', 0)})

8
nova/db/sqlalchemy/models.py
View File

@ -1453,10 +1453,10 @@ class PciDevice(BASE, NovaBase, models.SoftDeleteMixin):
parent_addr = Column(String(12), nullable=True)
instance = orm.relationship(Instance, backref="pci_devices",
foreign_keys=instance_uuid,
primaryjoin='and_('
'PciDevice.instance_uuid == Instance.uuid,'
'PciDevice.deleted == 0)')
foreign_keys=instance_uuid,
primaryjoin='and_('
'PciDevice.instance_uuid == Instance.uuid,'
'PciDevice.deleted == 0)')
class Tag(BASE, models.ModelBase):

2
nova/exception.py
View File

@ -1713,7 +1713,7 @@ class PciRequestAliasNotDefined(NovaException):
class PciConfigInvalidWhitelist(Invalid):
msg_fmt = _("Invalid PCI devices Whitelist config %(reason)s")
msg_fmt = _("Invalid PCI devices Whitelist config: %(reason)s")
# Cannot be templated, msg needs to be constructed when raised.

6
nova/objects/pci_device.py
View File

@ -303,9 +303,9 @@ class PciDevice(base.NovaPersistentObject, base.NovaObject):
parent.status = fields.PciDeviceStatus.UNCLAIMABLE
else:
LOG.debug('Physical function addr: %(pf_addr)s parent of '
'VF addr: %(vf_addr)s was not found',
{'pf_addr': self.parent_addr,
'vf_addr': self.address})
'VF addr: %(vf_addr)s was not found',
{'pf_addr': self.parent_addr,
'vf_addr': self.address})
self.status = fields.PciDeviceStatus.CLAIMED
self.instance_uuid = instance_uuid

67
nova/pci/devspec.py
View File

@ -31,22 +31,6 @@ ANY = '*'
REGEX_ANY = '.*'
def get_pci_dev_info(pci_obj, property, max, hex_value):
a = getattr(pci_obj, property)
if a == ANY:
return
try:
v = int(a, 16)
except ValueError:
raise exception.PciConfigInvalidWhitelist(
reason = "invalid %s %s" % (property, a))
if v > max:
raise exception.PciConfigInvalidWhitelist(
reason=_("invalid %(property)s %(attr)s") %
{'property': property, 'attr': a})
setattr(pci_obj, property, hex_value % v)
@six.add_metaclass(abc.ABCMeta)
class PciAddressSpec(object):
"""Abstract class for all PCI address spec styles
@ -65,6 +49,23 @@ class PciAddressSpec(object):
all(c in string.hexdigits for c in self.slot),
all(c in string.hexdigits for c in self.func)])
def _set_pci_dev_info(self, prop, maxval, hex_value):
a = getattr(self, prop)
if a == ANY:
return
try:
v = int(a, 16)
except ValueError:
raise exception.PciConfigInvalidWhitelist(
reason=_("property %(property)s ('%(attr)s') does not parse "
"as a hex number.") % {'property': prop, 'attr': a})
if v > maxval:
raise exception.PciConfigInvalidWhitelist(
reason=_("property %(property)s (%(attr)s) is greater than "
"the maximum allowable value (%(max)X).") %
{'property': prop, 'attr': a, 'max': maxval})
setattr(self, prop, hex_value % v)
class PhysicalPciAddress(PciAddressSpec):
"""Manages the address fields for a fully-qualified PCI address.
@ -82,10 +83,10 @@ class PhysicalPciAddress(PciAddressSpec):
else:
self.domain, self.bus, self.slot, self.func = (
utils.get_pci_address_fields(pci_addr))
get_pci_dev_info(self, 'func', MAX_FUNC, '%1x')
get_pci_dev_info(self, 'domain', MAX_DOMAIN, '%04x')
get_pci_dev_info(self, 'bus', MAX_BUS, '%02x')
get_pci_dev_info(self, 'slot', MAX_SLOT, '%02x')
self._set_pci_dev_info('func', MAX_FUNC, '%1x')
self._set_pci_dev_info('domain', MAX_DOMAIN, '%04x')
self._set_pci_dev_info('bus', MAX_BUS, '%02x')
self._set_pci_dev_info('slot', MAX_SLOT, '%02x')
except (KeyError, ValueError):
raise exception.PciDeviceWrongAddressFormat(address=pci_addr)
@ -115,7 +116,7 @@ class PciAddressGlobSpec(PciAddressSpec):
dbs, sep, func = pci_addr.partition('.')
if func:
self.func = func.strip()
get_pci_dev_info(self, 'func', MAX_FUNC, '%01x')
self._set_pci_dev_info('func', MAX_FUNC, '%01x')
if dbs:
dbs_fields = dbs.split(':')
if len(dbs_fields) > 3:
@ -127,9 +128,9 @@ class PciAddressGlobSpec(PciAddressSpec):
dbs_all.extend(dbs_fields)
dbs_checked = [s.strip() or ANY for s in dbs_all]
self.domain, self.bus, self.slot = dbs_checked
get_pci_dev_info(self, 'domain', MAX_DOMAIN, '%04x')
get_pci_dev_info(self, 'bus', MAX_BUS, '%02x')
get_pci_dev_info(self, 'slot', MAX_SLOT, '%02x')
self._set_pci_dev_info('domain', MAX_DOMAIN, '%04x')
self._set_pci_dev_info('bus', MAX_BUS, '%02x')
self._set_pci_dev_info('slot', MAX_SLOT, '%02x')
def match(self, phys_pci_addr):
conditions = [
@ -176,7 +177,7 @@ class WhitelistPciAddress(object):
This class checks the address fields of the pci.passthrough_whitelist
configuration option, validating the address fields.
Example config are:
Example configs:
| [pci]
| passthrough_whitelist = {"address":"*:0a:00.*",
@ -226,8 +227,8 @@ class WhitelistPciAddress(object):
# Try to match on the parent PCI address if the PciDeviceSpec is a
# PF (sriov is available) and the device to match is a VF. This
# makes possible to specify the PCI address of a PF in the
# pci_passthrough_whitelist to match any of it's VFs PCI devices.
# makes it possible to specify the PCI address of a PF in the
# pci.passthrough_whitelist to match any of its VFs' PCI addresses.
if self.is_physical_function and pci_phys_addr:
pci_phys_addr_obj = PhysicalPciAddress(pci_phys_addr)
if self.pci_address_spec.match(pci_phys_addr_obj):
@ -238,7 +239,7 @@ class WhitelistPciAddress(object):
return self.pci_address_spec.match(pci_addr_obj)
class PciDeviceSpec(object):
class PciDeviceSpec(PciAddressSpec):
def __init__(self, dev_spec):
self.tags = dev_spec
self._init_dev_details()
@ -253,8 +254,8 @@ class PciDeviceSpec(object):
self.dev_name = self.tags.pop("devname", None)
self.vendor_id = self.vendor_id.strip()
get_pci_dev_info(self, 'vendor_id', MAX_VENDOR_ID, '%04x')
get_pci_dev_info(self, 'product_id', MAX_PRODUCT_ID, '%04x')
self._set_pci_dev_info('vendor_id', MAX_VENDOR_ID, '%04x')
self._set_pci_dev_info('product_id', MAX_PRODUCT_ID, '%04x')
if self.address and self.dev_name:
raise exception.PciDeviceInvalidDeviceName()
@ -281,9 +282,9 @@ class PciDeviceSpec(object):
def match_pci_obj(self, pci_obj):
return self.match({'vendor_id': pci_obj.vendor_id,
'product_id': pci_obj.product_id,
'address': pci_obj.address,
'parent_addr': pci_obj.parent_addr})
'product_id': pci_obj.product_id,
'address': pci_obj.address,
'parent_addr': pci_obj.parent_addr})
def get_tags(self):
return self.tags

13
nova/pci/manager.py
View File

@ -38,14 +38,13 @@ class PciDevTracker(object):
It's called by compute node resource tracker to allocate and free
devices to/from instances, and to update the available pci passthrough
devices information from hypervisor periodically.
device information from the hypervisor periodically.
`pci_devs` attribute of this class is the in-memory "master copy" of all
devices on each compute host, and all data changes that happen when
claiming/allocating/freeing
devices HAVE TO be made against instances contained in `pci_devs` list,
because they are periodically flushed to the DB when the save()
method is called.
The `pci_devs` attribute of this class is the in-memory "master copy" of
all devices on each compute host, and all data changes that happen when
claiming/allocating/freeing devices HAVE TO be made against instances
contained in `pci_devs` list, because they are periodically flushed to the
DB when the save() method is called.
It is unsafe to fetch PciDevice objects elsewhere in the code for update
purposes as those changes will end up being overwritten when the `pci_devs`

20
nova/pci/request.py
View File

@ -17,24 +17,24 @@
| [pci]
| alias = '{
| "name": "QuicAssist",
| "name": "QuickAssist",
| "product_id": "0443",
| "vendor_id": "8086",
| "device_type": "type-PCI",
| }'
Aliases with the same name and the same device_type are OR operation::
Aliases with the same name and the same device_type are ORed::
| [pci]
| alias = '{
| "name": "QuicAssist",
| "name": "QuickAssist",
| "product_id": "0442",
| "vendor_id": "8086",
| "device_type": "type-PCI",
| }'
These 2 aliases define a device request meaning: vendor_id is "8086" and
product id is "0442" or "0443".
These two aliases define a device request meaning: vendor_id is "8086" and
product_id is "0442" or "0443".
"""
@ -156,11 +156,11 @@ def get_pci_requests_from_flavor(flavor):
'alias_name_x:count, alias_name_y:count, ... '. The alias_name is
defined in 'pci.alias' configurations.
The flavor's requirement is translated into pci requests list,
each entry in the list is a dictionary. The dictionary has
three keys. The 'specs' gives the pci device properties
requirement, the 'count' gives the number of devices, and the
optional 'alias_name' is the corresponding alias definition name.
The flavor's requirement is translated into a pci requests list.
Each entry in the list is a dict-ish object with three keys/attributes.
The 'specs' gives the pci device properties requirement, the 'count' gives
the number of devices, and the optional 'alias_name' is the corresponding
alias definition name.
Example:
Assume alias configuration is::

14
nova/pci/stats.py
View File

@ -156,12 +156,12 @@ class PciDeviceStats(object):
# Failed to allocate the required number of devices
# Return the devices already allocated back to their pools
if sum([pool['count'] for pool in pools]) < count:
LOG.error("Failed to allocate PCI devices for instance."
" Unassigning devices back to pools."
" This should not happen, since the scheduler"
" should have accurate information, and allocation"
" during claims is controlled via a hold"
" on the compute node semaphore")
LOG.error("Failed to allocate PCI devices for instance. "
"Unassigning devices back to pools. "
"This should not happen, since the scheduler "
"should have accurate information, and allocation "
"during claims is controlled via a hold "
"on the compute node semaphore.")
for d in range(len(alloc_devices)):
self.add_device(alloc_devices.pop())
return None
@ -223,7 +223,7 @@ class PciDeviceStats(object):
def _filter_non_requested_pfs(self, request, matching_pools):
# Remove SRIOV_PFs from pools, unless it has been explicitly requested
# This is especially needed in cases where PFs and VFs has the same
# This is especially needed in cases where PFs and VFs have the same
# product_id.
if all(spec.get('dev_type') != fields.PciDeviceType.SRIOV_PF for
spec in request.spec):

25
nova/pci/utils.py
View File

@ -72,12 +72,26 @@ def parse_address(address):
def get_pci_address_fields(pci_addr):
"""Parse a fully-specified PCI device address.
Does not validate that the components are valid hex or wildcard values.
:param pci_addr: A string of the form "<domain>:<bus>:<slot>.<function>".
:return: A 4-tuple of strings ("<domain>", "<bus>", "<slot>", "<function>")
"""
dbs, sep, func = pci_addr.partition('.')
domain, bus, slot = dbs.split(':')
return (domain, bus, slot, func)
return domain, bus, slot, func
def get_pci_address(domain, bus, slot, func):
"""Assembles PCI address components into a fully-specified PCI address.
Does not validate that the components are valid hex or wildcard values.
:param domain, bus, slot, func: Hex or wildcard strings.
:return: A string of the form "<domain>:<bus>:<slot>.<function>".
"""
return '%s:%s:%s.%s' % (domain, bus, slot, func)
@ -103,7 +117,6 @@ def is_physical_function(domain, bus, slot, function):
dev_path = "/sys/bus/pci/devices/%(d)s:%(b)s:%(s)s.%(f)s/" % {
"d": domain, "b": bus, "s": slot, "f": function}
if os.path.isdir(dev_path):
sriov_totalvfs = 0
try:
with open(dev_path + _SRIOV_TOTALVFS) as fd:
sriov_totalvfs = int(fd.read())
@ -119,12 +132,12 @@ def _get_sysfs_netdev_path(pci_addr, pf_interface):
Assumes a networking device - will not check for the existence of the path.
"""
if pf_interface:
return "/sys/bus/pci/devices/%s/physfn/net" % (pci_addr)
return "/sys/bus/pci/devices/%s/net" % (pci_addr)
return "/sys/bus/pci/devices/%s/physfn/net" % pci_addr
return "/sys/bus/pci/devices/%s/net" % pci_addr
def get_ifname_by_pci_address(pci_addr, pf_interface=False):
"""Get the interface name based on a VF's pci address
"""Get the interface name based on a VF's pci address.
The returned interface name is either the parent PF's or that of the VF
itself based on the argument of pf_interface.
@ -138,7 +151,7 @@ def get_ifname_by_pci_address(pci_addr, pf_interface=False):
def get_mac_by_pci_address(pci_addr, pf_interface=False):
"""Get the MAC address of the nic based on it's PCI address
"""Get the MAC address of the nic based on its PCI address.
Raises PciDeviceNotFoundById in case the pci device is not a NIC
"""

25
nova/pci/whitelist.py
View File

@ -26,15 +26,16 @@ CONF = nova.conf.CONF
class Whitelist(object):
"""White list class to decide assignable pci devices.
"""White list class to represent assignable pci devices.
Not all devices on compute node can be assigned to guest, the
cloud administrator decides the devices that can be assigned
based on vendor_id or product_id etc. If no white list specified,
no device will be assignable.
Not all devices on a compute node can be assigned to a guest. The
cloud administrator decides which devices can be assigned
based on vendor_id or product_id, etc. If no white list is specified,
no devices will be assignable.
"""
def _parse_white_list_from_config(self, whitelists):
@staticmethod
def _parse_white_list_from_config(whitelists):
"""Parse and validate the pci whitelist from the nova config."""
specs = []
for jsonspec in whitelists:
@ -64,14 +65,16 @@ class Whitelist(object):
def __init__(self, whitelist_spec=None):
"""White list constructor
For example, followed json string specifies that devices whose
For example, the following json string specifies that devices whose
vendor_id is '8086' and product_id is '1520' can be assigned
to guest.
to guests.
'[{"product_id":"1520", "vendor_id":"8086"}]'
:param whitelist_spec: A json string for a list of dictionaries,
each dictionary specifies the pci device
properties requirement.
:param whitelist_spec: A json string for a dictionary or list thereof.
Each dictionary specifies the pci device
properties requirement. See the definition of
passthrough_whitelist in nova.conf.pci for
details and examples.
"""
super(Whitelist, self).__init__()
if whitelist_spec:

4
nova/scheduler/host_manager.py
View File

@ -225,7 +225,7 @@ class HostState(object):
self.updated = compute.updated_at
self.numa_topology = compute.numa_topology
self.pci_stats = pci_stats.PciDeviceStats(
compute.pci_device_pools)
stats=compute.pci_device_pools)
# All virt drivers report host_ip
self.host_ip = compute.host_ip
@ -264,7 +264,7 @@ class HostState(object):
@set_update_time_on_success
def _locked(self, spec_obj):
# Scheduler API is inherently multi-threaded as every incoming RPC
# message will be dispatched in it's own green thread. So the
# message will be dispatched in its own green thread. So the
# shared host state should be consumed in a consistent way to make
# sure its data is valid under concurrent write operations.
self._locked_consume_from_request(spec_obj)

35
nova/tests/unit/pci/test_devspec.py
View File

@ -210,7 +210,8 @@ class PciAddressTestCase(test.NoDBTestCase):
pci_info = {"address": "0000:h4.12:6", "physical_network": "hr_net"}
exc = self.assertRaises(exception.PciConfigInvalidWhitelist,
devspec.PciDeviceSpec, pci_info)
msg = ('Invalid PCI devices Whitelist config invalid func 12:6')
msg = ("Invalid PCI devices Whitelist config: property func ('12:6') "
"does not parse as a hex number.")
self.assertEqual(msg, six.text_type(exc))
def test_max_func(self):
@ -218,8 +219,9 @@ class PciAddressTestCase(test.NoDBTestCase):
"physical_network": "hr_net"}
exc = self.assertRaises(exception.PciConfigInvalidWhitelist,
devspec.PciDeviceSpec, pci_info)
msg = ('Invalid PCI devices Whitelist config invalid func %x'
% (devspec.MAX_FUNC + 1))
msg = ('Invalid PCI devices Whitelist config: property func (%x) is '
'greater than the maximum allowable value (%x).'
% (devspec.MAX_FUNC + 1, devspec.MAX_FUNC))
self.assertEqual(msg, six.text_type(exc))
def test_max_domain(self):
@ -227,8 +229,9 @@ class PciAddressTestCase(test.NoDBTestCase):
"physical_network": "hr_net"}
exc = self.assertRaises(exception.PciConfigInvalidWhitelist,
devspec.PciDeviceSpec, pci_info)
msg = ('Invalid PCI devices Whitelist config invalid domain %x'
% (devspec.MAX_DOMAIN + 1))
msg = ('Invalid PCI devices Whitelist config: property domain (%X) '
'is greater than the maximum allowable value (%X).'
% (devspec.MAX_DOMAIN + 1, devspec.MAX_DOMAIN))
self.assertEqual(msg, six.text_type(exc))
def test_max_bus(self):
@ -236,8 +239,9 @@ class PciAddressTestCase(test.NoDBTestCase):
"physical_network": "hr_net"}
exc = self.assertRaises(exception.PciConfigInvalidWhitelist,
devspec.PciDeviceSpec, pci_info)
msg = ('Invalid PCI devices Whitelist config invalid bus %x'
% (devspec.MAX_BUS + 1))
msg = ('Invalid PCI devices Whitelist config: property bus (%X) is '
'greater than the maximum allowable value (%X).'
% (devspec.MAX_BUS + 1, devspec.MAX_BUS))
self.assertEqual(msg, six.text_type(exc))
def test_max_slot(self):
@ -245,8 +249,9 @@ class PciAddressTestCase(test.NoDBTestCase):
"physical_network": "hr_net"}
exc = self.assertRaises(exception.PciConfigInvalidWhitelist,
devspec.PciDeviceSpec, pci_info)
msg = ('Invalid PCI devices Whitelist config invalid slot %x'
% (devspec.MAX_SLOT + 1))
msg = ('Invalid PCI devices Whitelist config: property slot (%X) is '
'greater than the maximum allowable value (%X).'
% (devspec.MAX_SLOT + 1, devspec.MAX_SLOT))
self.assertEqual(msg, six.text_type(exc))
def test_address_is_undefined(self):
@ -380,8 +385,10 @@ class PciDevSpecTestCase(test.NoDBTestCase):
"product_id": "5057", "physical_network": "hr_net"}
exc = self.assertRaises(exception.PciConfigInvalidWhitelist,
devspec.PciDeviceSpec, pci_info)
self.assertEqual("Invalid PCI devices Whitelist config "
"invalid vendor_id 80860", six.text_type(exc))
self.assertEqual(
"Invalid PCI devices Whitelist config: property vendor_id (80860) "
"is greater than the maximum allowable value (FFFF).",
six.text_type(exc))
def test_invalid_product_id(self):
pci_info = {"vendor_id": "8086", "address": "*: *: *.5",
@ -394,8 +401,10 @@ class PciDevSpecTestCase(test.NoDBTestCase):
"product_id": "50570", "physical_network": "hr_net"}
exc = self.assertRaises(exception.PciConfigInvalidWhitelist,
devspec.PciDeviceSpec, pci_info)
self.assertEqual("Invalid PCI devices Whitelist config "
"invalid product_id 50570", six.text_type(exc))
self.assertEqual(
"Invalid PCI devices Whitelist config: property product_id "
"(50570) is greater than the maximum allowable value (FFFF).",
six.text_type(exc))
def test_devname_and_address(self):
pci_info = {"devname": "eth0", "vendor_id": "8086",