openstack/nova-powervm
Code Issues Proposed changes

Better config option validation and testing

Browse Source 
Added bounds checking to integer options and choice values for
string options where certain keywords are expected.  Also added
code to ignore case differences for expected keywords.

Change-Id: I7adb4ce8f1f1c2359b210296aaa52e1f0a80417e
This commit is contained in:
Kyle L. Henderson 2016-01-15 16:09:13 -06:00
parent f4f47699c3
commit 971d17afca
4 changed files with 127 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
nova_powervm/conf/powervm.py
View File

@ -35,7 +35,7 @@ powervm_opts = [
help='Factor used to calculate the processor units per vcpu. '
'Valid values are: 0.05 - 1.0'),
cfg.IntOpt('uncapped_proc_weight',
default=64,
default=64, min=1, max=255,
help='The processor weight to assign to newly created VMs. '
'Value should be between 1 and 255. Represents how '
'aggressively LPARs grab CPU when unused cycles are '
@ -46,7 +46,7 @@ powervm_opts = [
'to store the config drive metadata that will be attached '
'to VMs.'),
cfg.IntOpt('vopt_media_rep_size',
default=1,
default=1, min=1,
help='The size of the media repository (in GB) for the '
'metadata for config drive. Only used if the media '
'repository needs to be created.'),
@ -55,6 +55,7 @@ powervm_opts = [
help='The location where the config drive ISO files should be '
'built.'),
cfg.StrOpt('disk_driver',
choices=['localdisk', 'ssp'], ignore_case=True,
default='localdisk',
help='The disk driver to use for PowerVM disks. '
'Valid options are: localdisk, ssp'),
@ -90,10 +91,15 @@ ssp_opts = [
vol_adapter_opts = [
cfg.StrOpt('fc_attach_strategy',
choices=['vscsi', 'npiv'], ignore_case=True,
default='vscsi',
help='The Fibre Channel Volume Strategy defines how FC Cinder '
'volumes should be attached to the Virtual Machine. The '
'options are: npiv or vscsi.'),
'options are: npiv or vscsi. If npiv is selected then '
'the ports_per_fabric and fabrics option should be '
'specified and at least one fabric_X_port_wwpns option '
'(where X corresponds to the fabric name) must be '
'specified.'),
cfg.StrOpt('fc_npiv_adapter_api',
default='nova_powervm.virt.powervm.volume.npiv.'
'NPIVVolumeAdapter',
@ -104,7 +110,8 @@ vol_adapter_opts = [
'VscsiVolumeAdapter',
help='Volume Adapter API to connect FC volumes through Virtual '
'I/O Server using PowerVM vSCSI connection mechanism.'),
cfg.IntOpt('vscsi_vios_connections_required', default=1,
cfg.IntOpt('vscsi_vios_connections_required',
default=1, min=1,
help='Indicates a minimum number of Virtual I/O Servers that '
'are required to support a Cinder volume attach with the '
'vSCSI volume connector.')
@ -113,7 +120,8 @@ vol_adapter_opts = [
# NPIV Options. Only applicable if the 'fc_attach_strategy' is set to 'npiv'.
# Otherwise this section can be ignored.
npiv_opts = [
cfg.IntOpt('ports_per_fabric', default=1,
cfg.IntOpt('ports_per_fabric',
default=1, min=1,
help='The number of physical ports that should be connected '
'directly to the Virtual Machine, per fabric. '
'Example: 2 fabrics and ports_per_fabric set to 2 will '

99
nova_powervm/tests/conf/test_conf.py
View File

@ -42,6 +42,105 @@ class TestConf(test.TestCase):
self.assertEqual(1, CONF.powervm.ports_per_fabric)
class TestConfBounds(test.TestCase):
def setUp(self):
super(TestConfBounds, self).setUp()
def _bounds_test(self, should_pass, opts, **kwargs):
"""Test the bounds of an option."""
# Use the Oslo fixture to create a temporary conf object
with oslo_config.fixture.Config(oslo_config.cfg.ConfigOpts()) as fx:
# Load the raw values
fx.load_raw_values(group='powervm', **kwargs)
# Register the options
fx.register_opts(opts, group='powervm')
# For each kwarg option passed, validate it.
for kw in kwargs:
if not should_pass:
# Reference the option to cause a bounds exception
self.assertRaises(oslo_config.cfg.ConfigFileValueError,
lambda: fx.conf.powervm[kw])
else:
# It's expected to succeed
fx.conf.powervm[kw]
def test_bounds(self):
# Uncapped proc weight
self._bounds_test(False, cfg.powervm.powervm_opts,
uncapped_proc_weight=0)
self._bounds_test(False, cfg.powervm.powervm_opts,
uncapped_proc_weight=256)
self._bounds_test(True, cfg.powervm.powervm_opts,
uncapped_proc_weight=200)
# vopt media repo size
self._bounds_test(False, cfg.powervm.powervm_opts,
vopt_media_rep_size=0)
self._bounds_test(True, cfg.powervm.powervm_opts,
vopt_media_rep_size=10)
# vscsi connections
self._bounds_test(False, cfg.powervm.vol_adapter_opts,
vscsi_vios_connections_required=0)
self._bounds_test(True, cfg.powervm.vol_adapter_opts,
vscsi_vios_connections_required=2)
# ports per fabric
self._bounds_test(False, cfg.powervm.npiv_opts,
ports_per_fabric=0)
self._bounds_test(True, cfg.powervm.npiv_opts,
ports_per_fabric=2)
class TestConfChoices(test.TestCase):
def setUp(self):
super(TestConfChoices, self).setUp()
def _choice_test(self, invalid_choice, valid_choices, opts, option,
ignore_case=True):
"""Test the choices of an option."""
def _setup(fx, value):
# Load the raw values
fx.load_raw_values(group='powervm', **{option: value})
# Register the options
fx.register_opts(opts, group='powervm')
def _build_list():
for val in valid_choices:
yield val
yield val.lower()
yield val.upper()
if ignore_case:
# We expect to be able to ignore upper/lower case, so build a list
# of possibilities and ensure we do ignore them.
valid_choices = [x for x in _build_list()]
if invalid_choice:
# Use the Oslo fixture to create a temporary conf object
with oslo_config.fixture.Config(oslo_config.cfg.ConfigOpts()
) as fx:
_setup(fx, invalid_choice)
# Reference the option to cause an exception
self.assertRaises(oslo_config.cfg.ConfigFileValueError,
lambda: fx.conf.powervm[option])
for choice in valid_choices:
# Use the Oslo fixture to create a temporary conf object
with oslo_config.fixture.Config(oslo_config.cfg.ConfigOpts()
) as fx:
_setup(fx, choice)
# It's expected to succeed
fx.conf.powervm[option]
def test_choices(self):
# Disk driver
self._choice_test('bad_driver', ['localdisk', 'ssp'],
cfg.powervm.powervm_opts, 'disk_driver')
# FC attachment
self._choice_test('bad_value', ['vscsi', 'npiv'],
cfg.powervm.vol_adapter_opts, 'fc_attach_strategy')
class TestConfDynamic(test.TestCase):
def setUp(self):
super(TestConfDynamic, self).setUp()

13
nova_powervm/tests/virt/powervm/test_driver.py
View File

@ -140,6 +140,14 @@ class TestPowerVMDriver(test.TestCase):
self.assertIsNotNone(vol_connector['wwpns'])
self.assertIsNotNone(vol_connector['host'])
def test_get_disk_adapter(self):
# Ensure we can handle upper case option and we instantiate the class
self.flags(disk_driver='LoCaLDisK', group='powervm')
self.drv.disk_dvr = None
self.drv._get_disk_adapter()
# The local disk driver has been mocked, so we just compare the name
self.assertIn('LocalStorage()', str(self.drv.disk_dvr))
@mock.patch('nova_powervm.virt.powervm.vm.get_pvm_uuid')
@mock.patch('nova.context.get_admin_context')
def test_driver_ops(self, mock_get_ctx, mock_getuuid):
@ -683,6 +691,11 @@ class TestPowerVMDriver(test.TestCase):
pvm_vios.VIOS.xags.SCSI_MAPPING,
pvm_vios.VIOS.xags.FC_MAPPING]), set(xag))
# The vSCSI Volume attach - Ensure case insensitive.
self.flags(fc_attach_strategy='VSCSI', group='powervm')
xag = self.drv._get_inst_xag(mock.Mock(), [mock.Mock()])
self.assertEqual([pvm_vios.VIOS.xags.SCSI_MAPPING], xag)
def test_add_vol_conn_task(self):
bdm, vol_drv = mock.MagicMock(), mock.MagicMock()
flow = mock.Mock()

4
nova_powervm/virt/powervm/driver.py
View File

@ -123,7 +123,7 @@ class PowerVMDriver(driver.ComputeDriver):
'mp_uuid': self.mp_uuid}
self.disk_dvr = importutils.import_object_ns(
DISK_ADPT_NS, DISK_ADPT_MAPPINGS[CONF.powervm.disk_driver],
DISK_ADPT_NS, DISK_ADPT_MAPPINGS[CONF.powervm.disk_driver.lower()],
conn_info)
def _get_host_uuid(self):
@ -1553,7 +1553,7 @@ class PowerVMDriver(driver.ComputeDriver):
# If we have any volumes, add the volumes required mapping XAGs.
adp_type = vol_attach.FC_STRATEGY_MAPPING[
CONF.powervm.fc_attach_strategy]
CONF.powervm.fc_attach_strategy.lower()]
vol_cls = importutils.import_class(adp_type)
xags.update(set(vol_cls.min_xags()))
LOG.debug('Instance XAGs for VM %(inst)s is %(xags)s.',