Skip to content
Permalink
Browse files

libvirt : Fix slightly misleading parameter name, validate param

We actually need transport_iface, not transport as the param
(iscsi_tcp & iser are exceptions to this, where transport and
transport_iface are one and the same), so change it so that user is
not confused as to what must be provided. Also fixes a typo in
param help section.

Also added code to ensure that non-existing transport_iface is not
provided, in which case we we log a warning and fall back to the
default instead of failing as would have previously happened.

There is no change in functionality, this will just ensure that
documentation is not misleading.

Originally added in commit 554647a.

DocImpact - due to the renamed config option
Closes-Bug: #1420971
Change-Id: I6c190328803e8efcde34d522e2c1814ef6afc220
  • Loading branch information...
anish authored and mriedem committed Feb 7, 2015
1 parent 51cfe7d commit 3b8a2cf781d43d0b6937ec66aff5f37657f625ed
Showing with 76 additions and 11 deletions.
  1. +30 −4 nova/tests/unit/virt/libvirt/test_volume.py
  2. +46 −7 nova/virt/libvirt/volume.py
@@ -348,13 +348,35 @@ def test_libvirt_iscsi_get_host_device(self, transport=None):
expected_device = self.generate_device(transport, 1, False)
if transport:
self.stubs.Set(glob, 'glob', lambda x: [expected_device])
self.stubs.Set(libvirt_driver, '_validate_transport',
lambda x: True)
device = libvirt_driver._get_host_device(iscsi_properties)
self.assertEqual(expected_device, device)

def test_libvirt_iscsi_get_host_device_with_transport(self):
self.flags(iscsi_transport='fake_transport', group='libvirt')
self.flags(iscsi_iface='fake_transport', group='libvirt')
self.test_libvirt_iscsi_get_host_device('fake_transport')

@mock.patch.object(volume.utils, 'execute')
def test_libvirt_iscsi_validate_transport(self, mock_execute):
libvirt_driver = volume.LibvirtISCSIVolumeDriver(self.fake_conn)
sample_output = ('# BEGIN RECORD 2.0-872\n'
'iface.iscsi_ifacename = %s.fake_suffix\n'
'iface.net_ifacename = <empty>\n'
'iface.ipaddress = <empty>\n'
'iface.hwaddress = 00:53:00:00:53:00\n'
'iface.transport_name = %s\n'
'iface.initiatorname = <empty>\n'
'# END RECORD')
for tport in libvirt_driver.supported_transports:
mock_execute.return_value = (sample_output % (tport, tport), '')
self.assertTrue(libvirt_driver._validate_transport(
tport + '.fake_suffix'))

mock_execute.return_value = ("", 'iscsiadm: Could not '
'read iface fake_transport (6)')
self.assertFalse(libvirt_driver._validate_transport('fake_transport'))

def test_libvirt_iscsi_driver(self, transport=None):
# NOTE(vish) exists is to make driver assume connecting worked
self.stubs.Set(os.path, 'exists', lambda x: True)
@@ -364,6 +386,8 @@ def test_libvirt_iscsi_driver(self, transport=None):
if transport is not None:
self.stubs.Set(libvirt_driver, '_get_host_device',
lambda x: self.generate_device(transport, 1, False))
self.stubs.Set(libvirt_driver, '_validate_transport',
lambda x: True)
libvirt_driver.connect_volume(connection_info, self.disk_info)
libvirt_driver.disconnect_volume(connection_info, "vde")
expected_commands = [('iscsiadm', '-m', 'node', '-T', self.iqn,
@@ -386,7 +410,7 @@ def test_libvirt_iscsi_driver(self, transport=None):
self.assertEqual(expected_commands, self.executes)

def test_libvirt_iscsi_driver_with_transport(self):
self.flags(iscsi_transport='fake_transport', group='libvirt')
self.flags(iscsi_iface='fake_transport', group='libvirt')
self.test_libvirt_iscsi_driver('fake_transport')

def test_libvirt_iscsi_driver_still_in_use(self, transport=None):
@@ -397,6 +421,8 @@ def test_libvirt_iscsi_driver_still_in_use(self, transport=None):
if transport is not None:
self.stubs.Set(libvirt_driver, '_get_host_device',
lambda x: self.generate_device(transport, 1, False))
self.stubs.Set(libvirt_driver, '_validate_transport',
lambda x: True)
devs = [self.generate_device(transport, 2, False)]
self.stubs.Set(self.fake_conn, '_get_all_block_devices', lambda: devs)
vol = {'id': 1, 'name': self.name}
@@ -418,7 +444,7 @@ def test_libvirt_iscsi_driver_still_in_use(self, transport=None):
self.assertEqual(self.executes, expected_commands)

def test_libvirt_iscsi_driver_still_in_use_with_transport(self):
self.flags(iscsi_transport='fake_transport', group='libvirt')
self.flags(iscsi_iface='fake_transport', group='libvirt')
self.test_libvirt_iscsi_driver_still_in_use('fake_transport')

def test_libvirt_iscsi_driver_disconnect_multipath_error(self,
@@ -479,7 +505,7 @@ def test_libvirt_iscsi_driver_get_config(self, transport=None):
self.assertEqual(dev_path, tree.find('./source').get('dev'))

def test_libvirt_iscsi_driver_get_config_with_transport(self):
self.flags(iscsi_transport = 'fake_transport', group='libvirt')
self.flags(iscsi_iface = 'fake_transport', group='libvirt')
self.test_libvirt_iscsi_driver_get_config('fake_transport')

def test_libvirt_iscsi_driver_multipath_id(self):
@@ -102,11 +102,11 @@
'compute node'),
cfg.StrOpt('quobyte_client_cfg',
help='Path to a Quobyte Client configuration file.'),
cfg.StrOpt('iscsi_transport',
default=None,
help='The iSCSI transport to use to connect to target in case '
'offload support is desired. Supported transports are '
'be2iscsi, bnx2i, cxgb3i, cxgb4i, qla4xx and ocs. '
cfg.StrOpt('iscsi_iface',
deprecated_name='iscsi_transport',
help='The iSCSI transport iface to use to connect to target in '
'case offload support is desired. Supported transports '
'are be2iscsi, bnx2i, cxgb3i, cxgb4i, qla4xxx and ocs. '
'Default format is transport_name.hwaddress and can be '
'generated manually or via iscsiadm -m iface'),
# iser is also supported, but use LibvirtISERVolumeDriver
@@ -296,18 +296,57 @@ def disconnect_volume(self, connection_info, disk_dev):

class LibvirtISCSIVolumeDriver(LibvirtBaseVolumeDriver):
"""Driver to attach Network volumes to libvirt."""
supported_transports = ['be2iscsi', 'bnx2i', 'cxgb3i',
'cxgb4i', 'qla4xxx', 'ocs']

def __init__(self, connection):
super(LibvirtISCSIVolumeDriver, self).__init__(connection,
is_block_dev=True)
self.num_scan_tries = CONF.libvirt.num_iscsi_scan_tries
self.use_multipath = CONF.libvirt.iscsi_use_multipath
if CONF.libvirt.iscsi_transport:
self.transport = CONF.libvirt.iscsi_transport
if CONF.libvirt.iscsi_iface:
self.transport = CONF.libvirt.iscsi_iface
else:
self.transport = 'default'

def _get_transport(self):
if self._validate_transport(self.transport):
return self.transport
else:
return 'default'

def _validate_transport(self, transport_iface):
"""Check that given iscsi_iface uses only supported transports
Accepted transport names for provided iface param are
be2iscsi, bnx2i, cxgb3i, cxgb4i, qla4xxx and ocs. iSER uses it's
own separate driver. Note the difference between transport and
iface; unlike iscsi_tcp/iser, this is not one and the same for
offloaded transports, where the default format is
transport_name.hwaddress
"""
# We can support iser here as well, but currently reject it as the
# separate iser driver has not yet been deprecated.
if transport_iface == 'default':
return True
# Will return (6) if iscsi_iface file was not found, or (2) if iscsid
# could not be contacted
out = self._run_iscsiadm_bare(['-m',
'iface',
'-I',
transport_iface],
check_exit_code=[0, 2, 6])[0] or ""
LOG.debug("iscsiadm %(iface)s configuration: stdout=%(out)s",
{'iface': transport_iface, 'out': out})
for data in [line.split() for line in out.splitlines()]:
if data[0] == 'iface.transport_name':
if data[2] in self.supported_transports:
return True

LOG.warn(_LW("No useable transport found for iscsi iface %s. "
"Falling back to default transport"),
transport_iface)
return False

def _run_iscsiadm(self, iscsi_properties, iscsi_command, **kwargs):
check_exit_code = kwargs.pop('check_exit_code', 0)

0 comments on commit 3b8a2cf

Please sign in to comment.
You can’t perform that action at this time.