openstack
/
cinder
Code Issues Proposed changes

Move initiator_data access to helper methods 

This changes the flow for how a driver would access the initiator_data
information. Previously it was passed in at initialize_connection
and then a update dictionary merged in with the return of that method.
We will now just have drivers call helper util methods that can get
and set the data.

There are a few benefits of doing this:

* Drivers can synchronize access to the data, if they need to. It won’t
force any locks are anything on someone who doesn’t care. Future
iterations will make this do a conditional update so drivers won’t need
to use locks around it either.
* No more optional params on initialize_connection, meaning all drivers
have the same signature again.
* Less junk in the manager.
* Less db calls for everyone!

Change-Id: I9b90fafdbf290c7d7141b6ce4f83711f0fcd528f
Closes-Bug: #1578801
Closes-Bug: #1578864
This commit is contained in:
Patrick East 2016-05-05 16:29:56 -07:00
parent 8c3abfdfb0
commit 1058f298e5
7 changed files with 115 additions and 204 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
cinder/tests/unit/test_pure.py
View File

@ -1952,6 +1952,8 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
self.mock_config.use_chap_auth = False
self.driver = pure.PureISCSIDriver(configuration=self.mock_config)
self.driver._array = self.array
self.mock_utils = mock.Mock()
self.driver.driver_utils = self.mock_utils
def test_get_host(self):
good_host = PURE_HOST.copy()
@ -1985,7 +1987,7 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
ISCSI_CONNECTOR)
self.assertDictMatch(result, real_result)
mock_get_iscsi_ports.assert_called_with()
mock_connection.assert_called_with(VOLUME, ISCSI_CONNECTOR, None)
mock_connection.assert_called_with(VOLUME, ISCSI_CONNECTOR)
self.assert_error_propagates([mock_get_iscsi_ports, mock_connection],
self.driver.initialize_connection,
VOLUME, ISCSI_CONNECTOR)
@ -1998,8 +2000,6 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
chap_username = ISCSI_CONNECTOR["host"]
chap_password = "password"
mock_get_iscsi_ports.return_value = ISCSI_PORTS
initiator_update = [{"key": pure.CHAP_SECRET_KEY,
"value": chap_password}]
mock_connection.return_value = {
"vol": VOLUME["name"] + "-cinder",
"lun": 1,
@ -2016,15 +2016,7 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
# Branch where no credentials were generated
real_result = self.driver.initialize_connection(VOLUME,
ISCSI_CONNECTOR)
mock_connection.assert_called_with(VOLUME, ISCSI_CONNECTOR, None)
self.assertDictMatch(result, real_result)
# Branch where new credentials were generated
mock_connection.return_value["initiator_update"] = initiator_update
result["initiator_update"] = initiator_update
real_result = self.driver.initialize_connection(VOLUME,
ISCSI_CONNECTOR)
mock_connection.assert_called_with(VOLUME, ISCSI_CONNECTOR, None)
mock_connection.assert_called_with(VOLUME, ISCSI_CONNECTOR)
self.assertDictMatch(result, real_result)
self.assert_error_propagates([mock_get_iscsi_ports, mock_connection],
@ -2051,7 +2043,7 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
multipath_connector)
self.assertDictMatch(result, real_result)
mock_get_iscsi_ports.assert_called_with()
mock_connection.assert_called_with(VOLUME, multipath_connector, None)
mock_connection.assert_called_with(VOLUME, multipath_connector)
multipath_connector["multipath"] = False
self.driver.initialize_connection(VOLUME, multipath_connector)
@ -2088,7 +2080,7 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
# Branch where host already exists
mock_host.return_value = PURE_HOST
self.array.connect_host.return_value = {"vol": vol_name, "lun": 1}
real_result = self.driver._connect(VOLUME, ISCSI_CONNECTOR, None)
real_result = self.driver._connect(VOLUME, ISCSI_CONNECTOR)
self.assertEqual(result, real_result)
mock_host.assert_called_with(self.driver, self.array, ISCSI_CONNECTOR)
self.assertFalse(mock_generate.called)
@ -2098,7 +2090,7 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
# Branch where new host is created
mock_host.return_value = None
mock_generate.return_value = PURE_HOST_NAME
real_result = self.driver._connect(VOLUME, ISCSI_CONNECTOR, None)
real_result = self.driver._connect(VOLUME, ISCSI_CONNECTOR)
mock_host.assert_called_with(self.driver, self.array, ISCSI_CONNECTOR)
mock_generate.assert_called_with(HOSTNAME)
self.array.create_host.assert_called_with(PURE_HOST_NAME,
@ -2110,7 +2102,7 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
self.assert_error_propagates(
[mock_host, mock_generate, self.array.connect_host,
self.array.create_host], self.driver._connect, VOLUME,
ISCSI_CONNECTOR, None)
ISCSI_CONNECTOR)
self.mock_config.use_chap_auth = True
chap_user = ISCSI_CONNECTOR["host"]
@ -2119,7 +2111,8 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
# Branch where chap is used and credentials already exist
initiator_data = [{"key": pure.CHAP_SECRET_KEY,
"value": chap_password}]
self.driver._connect(VOLUME, ISCSI_CONNECTOR, initiator_data)
self.mock_utils.get_driver_initiator_data.return_value = initiator_data
self.driver._connect(VOLUME, ISCSI_CONNECTOR)
result["auth_username"] = chap_user
result["auth_password"] = chap_password
self.assertDictMatch(result, real_result)
@ -2129,10 +2122,11 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
# Branch where chap is used and credentials are generated
mock_gen_secret.return_value = chap_password
self.driver._connect(VOLUME, ISCSI_CONNECTOR, None)
self.mock_utils.get_driver_initiator_data.return_value = None
self.driver._connect(VOLUME, ISCSI_CONNECTOR)
result["auth_username"] = chap_user
result["auth_password"] = chap_password
result["initiator_update"] = {
expected_update = {
"set_values": {
pure.CHAP_SECRET_KEY: chap_password
},
@ -2141,6 +2135,10 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
self.array.set_host.assert_called_with(PURE_HOST_NAME,
host_user=chap_user,
host_password=chap_password)
self.mock_utils.save_driver_initiator_data.assert_called_with(
ISCSI_CONNECTOR['initiator'],
expected_update
)
@mock.patch(ISCSI_DRIVER_OBJ + "._get_host", autospec=True)
def test_connect_already_connected(self, mock_host):
@ -2153,7 +2151,7 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
code=400,
text="Connection already exists"
)
actual = self.driver._connect(VOLUME, ISCSI_CONNECTOR, None)
actual = self.driver._connect(VOLUME, ISCSI_CONNECTOR)
self.assertEqual(expected, actual)
self.assertTrue(self.array.connect_host.called)
self.assertTrue(self.array.list_volume_private_connections)
@ -2168,7 +2166,7 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
text="Connection already exists"
)
self.assertRaises(exception.PureDriverException, self.driver._connect,
VOLUME, ISCSI_CONNECTOR, None)
VOLUME, ISCSI_CONNECTOR)
self.assertTrue(self.array.connect_host.called)
self.assertTrue(self.array.list_volume_private_connections)
@ -2184,7 +2182,7 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
)
self.assertRaises(self.purestorage_module.PureHTTPError,
self.driver._connect, VOLUME,
ISCSI_CONNECTOR, None)
ISCSI_CONNECTOR)
self.assertTrue(self.array.connect_host.called)
self.assertTrue(self.array.list_volume_private_connections)

89
cinder/tests/unit/test_volume.py
View File

@ -2173,82 +2173,6 @@ class VolumeTestCase(BaseVolumeTestCase):
fake.VOLUME_ID,
connector)
@mock.patch.object(cinder.volume.targets.iscsi.ISCSITarget,
'_get_target_chap_auth')
@mock.patch.object(db, 'volume_admin_metadata_get')
@mock.patch.object(db, 'volume_update')
@mock.patch.object(db.sqlalchemy.api, 'volume_get')
@mock.patch.object(fake_driver.FakeISCSIDriver, 'initialize_connection')
@mock.patch.object(db, 'driver_initiator_data_get')
@mock.patch.object(db, 'driver_initiator_data_update')
def test_initialize_connection_initiator_data(self, mock_data_update,
mock_data_get,
mock_driver_init,
mock_volume_get,
mock_volume_update,
mock_metadata_get,
mock_get_target):
fake_admin_meta = {'fake-key': 'fake-value'}
fake_volume = {'volume_type_id': None,
'name': 'fake_name',
'host': 'fake_host',
'id': fake.VOLUME_ID,
'volume_admin_metadata': fake_admin_meta,
'encryption_key_id': ('d371e7bb-7392-4c27-'
'ac0b-ebd9f5d16078')}
mock_volume_get.return_value = fake_volume
mock_volume_update.return_value = fake_volume
mock_get_target.return_value = None
connector = {'ip': 'IP', 'initiator': 'INITIATOR'}
mock_driver_init.return_value = {
'driver_volume_type': 'iscsi',
'data': {'access_mode': 'rw',
'encrypted': False}
}
mock_data_get.return_value = []
conn_info = self.volume.initialize_connection(self.context, 'id',
connector)
# Asserts that if the driver sets the encrypted flag then the
# VolumeManager doesn't overwrite it regardless of what's in the
# volume for the encryption_key_id field.
self.assertFalse(conn_info['data']['encrypted'])
mock_driver_init.assert_called_with(fake_volume, connector)
data = [{'key': 'key1', 'value': 'value1'}]
mock_data_get.return_value = data
self.volume.initialize_connection(self.context, 'id', connector)
mock_driver_init.assert_called_with(fake_volume, connector, data)
update = {
'set_values': {
'foo': 'bar'
},
'remove_values': [
'foo',
'foo2'
]
}
mock_driver_init.return_value['initiator_update'] = update
self.volume.initialize_connection(self.context, 'id', connector)
mock_driver_init.assert_called_with(fake_volume, connector, data)
mock_data_update.assert_called_with(self.context, 'INITIATOR',
'FakeISCSIDriver', update)
connector['initiator'] = None
mock_data_update.reset_mock()
mock_data_get.reset_mock()
mock_driver_init.return_value['data'].pop('encrypted')
conn_info = self.volume.initialize_connection(self.context, 'id',
connector)
# Asserts that VolumeManager sets the encrypted flag if the driver
# doesn't set it.
self.assertTrue(conn_info['data']['encrypted'])
mock_driver_init.assert_called_with(fake_volume, connector)
self.assertFalse(mock_data_get.called)
self.assertFalse(mock_data_update.called)
def test_run_attach_detach_volume_for_instance(self):
"""Make sure volume can be attached and detached from instance."""
mountpoint = "/dev/sdf"
@ -4307,19 +4231,6 @@ class VolumeTestCase(BaseVolumeTestCase):
volume = db.volume_get(context.get_admin_context(), test_vol_id)
self.assertEqual('error', volume['status'])
def test__get_driver_initiator_data(self):
manager = vol_manager.VolumeManager()
data = manager._get_driver_initiator_data(None, {'key': 'val'})
self.assertIsNone(data)
connector = {'initiator': {'key': 'val'}}
self.assertRaises(exception.InvalidInput,
manager._get_driver_initiator_data,
None,
connector)
def test_cascade_delete_volume_with_snapshots(self):
"""Test volume deletion with dependent snapshots."""
volume = tests_utils.create_volume(self.context, **self.volume_params)

29
cinder/volume/driver.py
View File

@ -30,6 +30,7 @@ from cinder.i18n import _, _LE, _LW
from cinder.image import image_utils
from cinder import objects
from cinder import utils
from cinder.volume import driver_utils
from cinder.volume import rpcapi as volume_rpcapi
from cinder.volume import throttling
@ -316,6 +317,9 @@ class BaseVD(object):
self.configuration.append_config_values(iser_opts)
utils.setup_tracing(self.configuration.safe_get('trace_flags'))
self.driver_utils = driver_utils.VolumeDriverUtils(
self._driver_data_namespace(), self.db)
self._execute = execute
self._stats = {}
self._throttle = None
@ -337,6 +341,14 @@ class BaseVD(object):
# set True by manager after successful check_for_setup
self._initialized = False
def _driver_data_namespace(self):
namespace = self.__class__.__name__
if self.configuration:
namespace = self.configuration.safe_get('driver_data_namespace')
if not namespace:
namespace = self.configuration.safe_get('volume_backend_name')
return namespace
def _is_non_recoverable(self, err, non_recoverable_list):
for item in non_recoverable_list:
if item in err:
@ -1486,26 +1498,13 @@ class BaseVD(object):
return
@abc.abstractmethod
def initialize_connection(self, volume, connector, initiator_data=None):
def initialize_connection(self, volume, connector):
"""Allow connection to connector and return connection info.
:param volume: The volume to be attached
:param connector: Dictionary containing information about what is being
connected to.
:param initiator_data (optional): A dictionary of
driver_initiator_data
objects with key-value pairs that
have been saved for this initiator
by a driver in previous
initialize_connection calls.
:returns conn_info: A dictionary of connection information. This can
optionally include a "initiator_updates" field.
The "initiator_updates" field must be a dictionary containing a
"set_values" and/or "remove_values" field. The "set_values" field must
be a dictionary of key-value pairs to be set/updated in the db. The
"remove_values" field must be a list of keys, previously set with
"set_values", that will be deleted from the db.
:returns conn_info: A dictionary of connection information.
"""
return

63
cinder/volume/driver_utils.py Normal file
View File

@ -0,0 +1,63 @@
# Copyright (c) 2014 Pure Storage, Inc.
# All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
from oslo_log import log as logging
from cinder import context
from cinder import exception
from cinder.i18n import _LE
LOG = logging.getLogger(__name__)
class VolumeDriverUtils(object):
def __init__(self, namespace, db):
self._data_namespace = namespace
self._db = db
@staticmethod
def _get_context(ctxt):
if not ctxt:
return context.get_admin_context()
return ctxt
def get_driver_initiator_data(self, initiator, ctxt=None):
try:
return self._db.driver_initiator_data_get(
self._get_context(ctxt),
initiator,
self._data_namespace
)
except exception.CinderException:
LOG.exception(_LE("Failed to get driver initiator data for"
" initiator %(initiator)s and namespace"
" %(namespace)s"),
{'initiator': initiator,
'namespace': self._data_namespace})
raise
def save_driver_initiator_data(self, initiator, model_update, ctxt=None):
try:
self._db.driver_initiator_data_update(self._get_context(ctxt),
initiator,
self._data_namespace,
model_update)
except exception.CinderException:
LOG.exception(_LE("Failed to update initiator data for"
" initiator %(initiator)s and backend"
" %(backend)s"),
{'initiator': initiator,
'backend': self._data_namespace})
raise

28
cinder/volume/drivers/pure.py
View File

@ -408,7 +408,7 @@ class PureBaseVolumeDriver(san.SanDriver):
def create_export(self, context, volume, connector):
pass
def initialize_connection(self, volume, connector, initiator_data=None):
def initialize_connection(self, volume, connector):
"""Connect the volume to the specified initiator in Purity.
This implementation is specific to the host type (iSCSI, FC, etc).
@ -1493,9 +1493,9 @@ class PureISCSIDriver(PureBaseVolumeDriver, san.SanISCSIDriver):
return None
@pure_driver_debug_trace
def initialize_connection(self, volume, connector, initiator_data=None):
def initialize_connection(self, volume, connector):
"""Allow connection to connector and return connection info."""
connection = self._connect(volume, connector, initiator_data)
connection = self._connect(volume, connector)
target_ports = self._get_target_iscsi_ports()
multipath = connector.get("multipath", False)
@ -1557,8 +1557,8 @@ class PureISCSIDriver(PureBaseVolumeDriver, san.SanISCSIDriver):
def _generate_chap_secret():
return volume_utils.generate_password()
@classmethod
def _get_chap_credentials(cls, host, data):
def _get_chap_credentials(self, host, initiator):
data = self.driver_utils.get_driver_initiator_data(initiator)
initiator_updates = None
username = host
password = None
@ -1568,22 +1568,25 @@ class PureISCSIDriver(PureBaseVolumeDriver, san.SanISCSIDriver):
password = d["value"]
break
if not password:
password = cls._generate_chap_secret()
password = self._generate_chap_secret()
initiator_updates = {
"set_values": {
CHAP_SECRET_KEY: password
}
}
return username, password, initiator_updates
if initiator_updates:
self.driver_utils.save_driver_initiator_data(initiator,
initiator_updates)
return username, password
@utils.synchronized(CONNECT_LOCK_NAME, external=True)
def _connect(self, volume, connector, initiator_data):
def _connect(self, volume, connector):
"""Connect the host and volume; return dict describing connection."""
iqn = connector["initiator"]
if self.configuration.use_chap_auth:
(chap_username, chap_password, initiator_update) = \
self._get_chap_credentials(connector['host'], initiator_data)
(chap_username, chap_password) = \
self._get_chap_credentials(connector['host'], iqn)
current_array = self._get_current_array()
vol_name = self._get_vol_name(volume)
@ -1631,9 +1634,6 @@ class PureISCSIDriver(PureBaseVolumeDriver, san.SanISCSIDriver):
connection["auth_username"] = chap_username
connection["auth_password"] = chap_password
if initiator_update:
connection["initiator_update"] = initiator_update
return connection
@ -1663,7 +1663,7 @@ class PureFCDriver(PureBaseVolumeDriver, driver.FibreChannelDriver):
@fczm_utils.AddFCZone
@pure_driver_debug_trace
def initialize_connection(self, volume, connector, initiator_data=None):
def initialize_connection(self, volume, connector):
"""Allow connection to connector and return connection info."""
current_array = self._get_current_array()
connection = self._connect(volume, connector)

9
cinder/volume/drivers/solidfire.py
View File

@ -1650,14 +1650,12 @@ class SolidFireDriver(san.SanISCSIDriver):
results['thinProvisioningPercent'])
self.cluster_stats = data
def initialize_connection(self, volume, connector, initiator_data=None):
def initialize_connection(self, volume, connector):
"""Initialize the connection and return connection info.
Optionally checks and utilizes volume access groups.
"""
properties = self._sf_initialize_connection(volume,
connector,
initiator_data)
properties = self._sf_initialize_connection(volume, connector)
properties['data']['discard'] = True
return properties
@ -1979,8 +1977,7 @@ class SolidFireISCSI(iscsi_driver.SanISCSITarget):
def terminate_connection(self, volume, connector, **kwargs):
pass
def _sf_initialize_connection(self, volume, connector,
initiator_data=None):
def _sf_initialize_connection(self, volume, connector):
"""Initialize the connection and return connection info.
Optionally checks and utilizes volume access groups.

59
cinder/volume/manager.py
View File

@ -1353,50 +1353,6 @@ class VolumeManager(manager.SchedulerDependentManager):
exc_info=True, resource={'type': 'image',
'id': image_id})
def _driver_data_namespace(self):
return self.driver.configuration.safe_get('driver_data_namespace') \
or self.driver.configuration.safe_get('volume_backend_name') \
or self.driver.__class__.__name__
def _get_driver_initiator_data(self, context, connector):
data = None
initiator = connector.get('initiator', False)
if initiator:
if not isinstance(initiator, six.string_types):
msg = _('Invalid initiator value received')
raise exception.InvalidInput(reason=msg)
namespace = self._driver_data_namespace()
try:
data = self.db.driver_initiator_data_get(
context,
initiator,
namespace
)
except exception.CinderException:
LOG.exception(_LE("Failed to get driver initiator data for"
" initiator %(initiator)s and namespace"
" %(namespace)s"),
{'initiator': initiator,
'namespace': namespace})
raise
return data
def _save_driver_initiator_data(self, context, connector, model_update):
if connector.get('initiator', False) and model_update:
namespace = self._driver_data_namespace()
try:
self.db.driver_initiator_data_update(context,
connector['initiator'],
namespace,
model_update)
except exception.CinderException:
LOG.exception(_LE("Failed to update initiator data for"
" initiator %(initiator)s and backend"
" %(backend)s"),
{'initiator': connector['initiator'],
'backend': namespace})
raise
def initialize_connection(self, context, volume_id, connector):
"""Prepare volume for connection from host represented by connector.
@ -1467,15 +1423,8 @@ class VolumeManager(manager.SchedulerDependentManager):
LOG.exception(_LE("Model update failed."), resource=volume)
raise exception.ExportFailure(reason=six.text_type(ex))
initiator_data = self._get_driver_initiator_data(context, connector)
try:
if initiator_data:
conn_info = self.driver.initialize_connection(volume,
connector,
initiator_data)
else:
conn_info = self.driver.initialize_connection(volume,
connector)
conn_info = self.driver.initialize_connection(volume, connector)
except Exception as err:
err_msg = (_("Driver initialize connection failed "
"(error: %(err)s).") % {'err': six.text_type(err)})
@ -1485,12 +1434,6 @@ class VolumeManager(manager.SchedulerDependentManager):
raise exception.VolumeBackendAPIException(data=err_msg)
initiator_update = conn_info.get('initiator_update', None)
if initiator_update:
self._save_driver_initiator_data(context, connector,
initiator_update)
del conn_info['initiator_update']
# Add qos_specs to connection info
typeid = volume['volume_type_id']
specs = None