Give more fine grained access to DriverInitiatorData

Previously we were setting a whole model update, whereas now we only
do one value at a time. This will allow for easier handling of doing
the insert if not exist and reporting back what happened.

The older method to do this was better suited to the model_update from
initialize_connection, but we don’t need a ‘all-in-one’ kind of method
anymore.

Change-Id: I57778f1807d1814c536d62757d6ad10c920b6101
Closes-Bug: #1588088

cinder/db/api.py

@@ -1100,14 +1100,24 @@ def get_booleans_for_table(table_name):
 ###################
 
 
-def driver_initiator_data_update(context, initiator, namespace, updates):
+def driver_initiator_data_insert_by_key(context, initiator,
-    """Create DriverPrivateData from the values dictionary."""
+                                        namespace, key, value):
-    return IMPL.driver_initiator_data_update(context, initiator,
+    """Updates DriverInitiatorData entry.
-                                             namespace, updates)
+
+    Sets the value for the specified key within the namespace.
+
+    If the entry already exists return False, if it inserted successfully
+    return True.
+    """
+    return IMPL.driver_initiator_data_insert_by_key(context,
+                                                    initiator,
+                                                    namespace,
+                                                    key,
+                                                    value)
 
 
 def driver_initiator_data_get(context, initiator, namespace):
-    """Query for an DriverPrivateData that has the specified key"""
+    """Query for an DriverInitiatorData that has the specified key"""
     return IMPL.driver_initiator_data_get(context, initiator, namespace)
 
 

cinder/db/sqlalchemy/api.py

@@ -4563,35 +4563,20 @@ def message_destroy(context, message):
 
 
 @require_context
-def driver_initiator_data_update(context, initiator, namespace, updates):
+def driver_initiator_data_insert_by_key(context, initiator, namespace,
-    session = get_session()
+                                        key, value):
-    with session.begin():
-        set_values = updates.get('set_values', {})
-        for key, value in set_values.items():
-            data = session.query(models.DriverInitiatorData).\
-                filter_by(initiator=initiator).\
-                filter_by(namespace=namespace).\
-                filter_by(key=key).\
-                first()
-
-            if data:
-                data.update({'value': value})
-                data.save(session=session)
-            else:
     data = models.DriverInitiatorData()
     data.initiator = initiator
     data.namespace = namespace
     data.key = key
     data.value = value
+    session = get_session()
+    try:
+        with session.begin():
             session.add(data)
-
+        return True
-        remove_values = updates.get('remove_values', [])
+    except db_exc.DBDuplicateEntry:
-        for key in remove_values:
+        return False
-            session.query(models.DriverInitiatorData).\
-                filter_by(initiator=initiator).\
-                filter_by(namespace=namespace).\
-                filter_by(key=key).\
-                delete()
 
 
 @require_context

cinder/tests/unit/test_db_api.py

@@ -2274,61 +2274,22 @@ class DBAPIDriverInitiatorDataTestCase(BaseTest):
     initiator = 'iqn.1993-08.org.debian:01:222'
     namespace = 'test_ns'
 
-    def test_driver_initiator_data_set_and_remove(self):
+    def _test_insert(self, key, value, expected_result=True):
-        data_key = 'key1'
+        result = db.driver_initiator_data_insert_by_key(
-        data_value = 'value1'
+            self.ctxt, self.initiator, self.namespace, key, value)
-        update = {
+        self.assertEqual(expected_result, result)
-            'set_values': {
-                data_key: data_value
-            }
-        }
 
-        db.driver_initiator_data_update(self.ctxt, self.initiator,
-                                        self.namespace, update)
         data = db.driver_initiator_data_get(self.ctxt, self.initiator,
                                             self.namespace)
+        self.assertEqual(data[0].key, key)
+        self.assertEqual(data[0].value, value)
 
-        self.assertIsNotNone(data)
+    def test_insert(self):
-        self.assertEqual(data_key, data[0]['key'])
+        self._test_insert('key1', 'foo')
-        self.assertEqual(data_value, data[0]['value'])
 
-        update = {'remove_values': [data_key]}
+    def test_insert_already_exists(self):
-
+        self._test_insert('key2', 'bar')
-        db.driver_initiator_data_update(self.ctxt, self.initiator,
+        self._test_insert('key2', 'bar', expected_result=False)
-                                        self.namespace, update)
-        data = db.driver_initiator_data_get(self.ctxt, self.initiator,
-                                            self.namespace)
-
-        self.assertIsNotNone(data)
-        self.assertEqual([], data)
-
-    def test_driver_initiator_data_no_changes(self):
-        db.driver_initiator_data_update(self.ctxt, self.initiator,
-                                        self.namespace, {})
-        data = db.driver_initiator_data_get(self.ctxt, self.initiator,
-                                            self.namespace)
-
-        self.assertIsNotNone(data)
-        self.assertEqual([], data)
-
-    def test_driver_initiator_data_update_existing_values(self):
-        data_key = 'key1'
-        data_value = 'value1'
-        update = {'set_values': {data_key: data_value}}
-        db.driver_initiator_data_update(self.ctxt, self.initiator,
-                                        self.namespace, update)
-        data_value = 'value2'
-        update = {'set_values': {data_key: data_value}}
-        db.driver_initiator_data_update(self.ctxt, self.initiator,
-                                        self.namespace, update)
-        data = db.driver_initiator_data_get(self.ctxt, self.initiator,
-                                            self.namespace)
-        self.assertEqual(data_value, data[0]['value'])
-
-    def test_driver_initiator_data_remove_not_existing(self):
-        update = {'remove_values': ['key_that_doesnt_exist']}
-        db.driver_initiator_data_update(self.ctxt, self.initiator,
-                                        self.namespace, update)
 
 
 class DBAPIImageVolumeCacheEntryTestCase(BaseTest):

cinder/tests/unit/test_pure.py

@@ -2098,18 +2098,15 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
         self.driver._connect(VOLUME, ISCSI_CONNECTOR)
         result["auth_username"] = chap_user
         result["auth_password"] = chap_password
-        expected_update = {
+
-            "set_values": {
-                pure.CHAP_SECRET_KEY: chap_password
-            },
-        }
         self.assertDictMatch(result, real_result)
         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(
+        self.mock_utils.insert_driver_initiator_data.assert_called_with(
             ISCSI_CONNECTOR['initiator'],
-            expected_update
+            pure.CHAP_SECRET_KEY,
+            chap_password
         )
 
     @mock.patch(ISCSI_DRIVER_OBJ + "._get_host", autospec=True)
@@ -2158,6 +2155,39 @@ class PureISCSIDriverTestCase(PureDriverTestCase):
         self.assertTrue(self.array.connect_host.called)
         self.assertTrue(self.array.list_volume_private_connections)
 
+    @mock.patch(ISCSI_DRIVER_OBJ + "._generate_chap_secret")
+    def test_get_chap_credentials_create_new(self, mock_generate_secret):
+        self.mock_utils.get_driver_initiator_data.return_value = []
+        host = 'host1'
+        expected_password = 'foo123'
+        mock_generate_secret.return_value = expected_password
+        self.mock_utils.insert_driver_initiator_data.return_value = True
+        username, password = self.driver._get_chap_credentials(host,
+                                                               INITIATOR_IQN)
+        self.assertEqual(host, username)
+        self.assertEqual(expected_password, password)
+        self.mock_utils.insert_driver_initiator_data.assert_called_once_with(
+            INITIATOR_IQN, pure.CHAP_SECRET_KEY, expected_password
+        )
+
+    @mock.patch(ISCSI_DRIVER_OBJ + "._generate_chap_secret")
+    def test_get_chap_credentials_create_new_fail_to_set(self,
+                                                         mock_generate_secret):
+        host = 'host1'
+        expected_password = 'foo123'
+        mock_generate_secret.return_value = 'badpassw0rd'
+        self.mock_utils.insert_driver_initiator_data.return_value = False
+        self.mock_utils.get_driver_initiator_data.side_effect = [
+            [],
+            [{'key': pure.CHAP_SECRET_KEY, 'value': expected_password}],
+            exception.PureDriverException(reason='this should never be hit'),
+        ]
+
+        username, password = self.driver._get_chap_credentials(host,
+                                                               INITIATOR_IQN)
+        self.assertEqual(host, username)
+        self.assertEqual(expected_password, password)
+
 
 class PureFCDriverTestCase(PureDriverTestCase):
 

cinder/volume/driver_utils.py

@@ -48,16 +48,25 @@ class VolumeDriverUtils(object):
                            'namespace': self._data_namespace})
             raise
 
-    def save_driver_initiator_data(self, initiator, model_update, ctxt=None):
+    def insert_driver_initiator_data(self, initiator, key, value, ctxt=None):
+        """Update the initiator data at key with value.
+
+        If the key has already been set to something return False, otherwise
+        if saved successfully return True.
+        """
         try:
-            self._db.driver_initiator_data_update(self._get_context(ctxt),
+            return self._db.driver_initiator_data_insert_by_key(
+                self._get_context(ctxt),
                 initiator,
                 self._data_namespace,
-                                                  model_update)
+                key,
+                value
+            )
         except exception.CinderException:
-            LOG.exception(_LE("Failed to update initiator data for"
+            LOG.exception(_LE("Failed to insert initiator data for"
                               " initiator %(initiator)s and backend"
-                              " %(backend)s"),
+                              " %(backend)s for key %(key)s."),
                           {'initiator': initiator,
-                           'backend': self._data_namespace})
+                           'backend': self._data_namespace,
+                           'key': key})
             raise

cinder/volume/drivers/pure.py

@@ -1552,26 +1552,27 @@ class PureISCSIDriver(PureBaseVolumeDriver, san.SanISCSIDriver):
     def _generate_chap_secret():
         return volume_utils.generate_password()
 
-    def _get_chap_credentials(self, host, initiator):
+    def _get_chap_secret_from_init_data(self, initiator):
         data = self.driver_utils.get_driver_initiator_data(initiator)
-        initiator_updates = None
-        username = host
-        password = None
         if data:
             for d in data:
                 if d["key"] == CHAP_SECRET_KEY:
-                    password = d["value"]
+                    return d["value"]
-                    break
+        return None
+
+    def _get_chap_credentials(self, host, initiator):
+        username = host
+        password = self._get_chap_secret_from_init_data(initiator)
         if not password:
             password = self._generate_chap_secret()
-            initiator_updates = {
+            success = self.driver_utils.insert_driver_initiator_data(
-                "set_values": {
+                initiator, CHAP_SECRET_KEY, password)
-                    CHAP_SECRET_KEY: password
+            if not success:
-                }
+                # The only reason the save would have failed is if someone
-            }
+                # else (read: another thread/instance of the driver) set
-        if initiator_updates:
+                # one before we did. In that case just do another query.
-            self.driver_utils.save_driver_initiator_data(initiator,
+                password = self._get_chap_secret_from_init_data(initiator)
-                                                         initiator_updates)
+
         return username, password
 
     @utils.synchronized(CONNECT_LOCK_NAME, external=True)