Refactor driver BDM attach() to cover all uses

Add separate flags to allow the execution to skip the
volume_api.check_attach step, and to be able to call the
driver.attach_volume if needed. This will make the method modular enough
to be used in all cases where we actually "attach" volumes to instances.

Check attach should always be done in the API, followed immediately by a
reservation, to minimize the chance of a race. However currently we
do the check_attach step on the compute side during boot, so it needs to
stay. Other operations like attach_volume will have it reserved causing
this check to fail on the compute side, so we lay down the groundwork to
be able to skip it when calling attach().

Likewise, when booting, we don't need to call the virt driver
attach_volume method, as all the devices will be taken care of by the
boot code path.

Part of blueprint: use-new-bdm-format-in-attach

Change-Id: Iff3890e39a8c75b1693df0918084ecf4492c4e91

nova/tests/virt/test_block_device.py

@@ -22,6 +22,7 @@ from nova.tests import matchers
 from nova.virt import block_device as driver_block_device
 from nova.virt import driver
 from nova.volume import cinder
+from nova.volume import encryptors
 
 
 class TestDriverBlockDevice(test.NoDBTestCase):
@@ -276,25 +277,57 @@ class TestDriverBlockDevice(test.NoDBTestCase):
         self.assertRaises(driver_block_device._InvalidType,
                           self.driver_classes['image'], bdm)
 
-    def _test_volume_attach(self, driver_bdm, bdm_dict, fake_volume):
+    def _test_volume_attach(self, driver_bdm, bdm_dict,
+                            fake_volume, check_attach=True,
+                            fail_check_attach=False, driver_attach=False,
+                            fail_driver_attach=False):
         elevated_context = self.context.elevated()
         self.stubs.Set(self.context, 'elevated',
                        lambda: elevated_context)
         self.mox.StubOutWithMock(driver_bdm._bdm_obj, 'save')
+        self.mox.StubOutWithMock(encryptors, 'get_encryption_metadata')
         instance = {'id': 'fake_id', 'uuid': 'fake_uuid'}
         connector = {'ip': 'fake_ip', 'host': 'fake_host'}
         connection_info = {'data': {}}
         expected_conn_info = {'data': {},
                               'serial': fake_volume['id']}
+        enc_data = {'fake': 'enc_data'}
 
         self.volume_api.get(self.context,
                             fake_volume['id']).AndReturn(fake_volume)
-        self.volume_api.check_attach(self.context, fake_volume,
+        if check_attach:
-                                instance=instance).AndReturn(None)
+            if not fail_check_attach:
+                self.volume_api.check_attach(self.context, fake_volume,
+                                    instance=instance).AndReturn(None)
+            else:
+                self.volume_api.check_attach(self.context, fake_volume,
+                                    instance=instance).AndRaise(
+                                            test.TestingException)
+                return instance, expected_conn_info
+
         self.virt_driver.get_volume_connector(instance).AndReturn(connector)
         self.volume_api.initialize_connection(
             elevated_context, fake_volume['id'],
             connector).AndReturn(connection_info)
+        if driver_attach:
+            encryptors.get_encryption_metadata(
+                    elevated_context, self.volume_api, fake_volume['id'],
+                    connection_info).AndReturn(enc_data)
+            if not fail_driver_attach:
+                self.virt_driver.attach_volume(
+                        elevated_context, expected_conn_info, instance,
+                        bdm_dict['device_name'],
+                        encryption=enc_data).AndReturn(None)
+            else:
+                self.virt_driver.attach_volume(
+                        elevated_context, expected_conn_info, instance,
+                        bdm_dict['device_name'],
+                        encryption=enc_data).AndRaise(test.TestingException)
+                self.volume_api.terminate_connection(
+                        elevated_context, fake_volume['id'],
+                        expected_conn_info).AndReturn(None)
+                return instance, expected_conn_info
+
         self.volume_api.attach(elevated_context, fake_volume['id'],
                           'fake_uuid', bdm_dict['device_name']).AndReturn(None)
         driver_bdm._bdm_obj.save(self.context).AndReturn(None)
@@ -315,6 +348,48 @@ class TestDriverBlockDevice(test.NoDBTestCase):
         self.assertThat(test_bdm['connection_info'],
                         matchers.DictMatches(expected_conn_info))
 
+    def check_volume_attach_check_attach_fails(self):
+        test_bdm = self.driver_classes['volume'](
+            self.volume_bdm)
+        volume = {'id': 'fake-volume-id-1'}
+
+        instance, _ = self._test_volume_attach(
+                test_bdm, self.volume_bdm, volume, fail_check_attach=True)
+        self.mox.ReplayAll()
+
+        self.asserRaises(test.TestingException, test_bdm.attach, self.context,
+                         instance, self.volume_api, self.virt_driver)
+
+    def test_volume_attach_no_check_driver_attach(self):
+        test_bdm = self.driver_classes['volume'](
+            self.volume_bdm)
+        volume = {'id': 'fake-volume-id-1'}
+
+        instance, expected_conn_info = self._test_volume_attach(
+                test_bdm, self.volume_bdm, volume, check_attach=False,
+                driver_attach=True)
+
+        self.mox.ReplayAll()
+
+        test_bdm.attach(self.context, instance,
+                        self.volume_api, self.virt_driver,
+                        do_check_attach=False, do_driver_attach=True)
+        self.assertThat(test_bdm['connection_info'],
+                        matchers.DictMatches(expected_conn_info))
+
+    def check_volume_attach_driver_attach_fails(self):
+        test_bdm = self.driver_classes['volume'](
+            self.volume_bdm)
+        volume = {'id': 'fake-volume-id-1'}
+
+        instance, _ = self._test_volume_attach(
+                test_bdm, self.volume_bdm, volume, fail_check_attach=True)
+        self.mox.ReplayAll()
+
+        self.asserRaises(test.TestingException, test_bdm.attach, self.context,
+                         instance, self.volume_api, self.virt_driver,
+                         do_driver_attach=True)
+
     def test_refresh_connection(self):
         test_bdm = self.driver_classes['snapshot'](
             self.snapshot_bdm)

nova/virt/block_device.py

@@ -17,9 +17,11 @@ import operator
 
 from nova import block_device
 from nova.objects import block_device as block_device_obj
+from nova.openstack.common import excutils
 from nova.openstack.common.gettextutils import _
 from nova.openstack.common import jsonutils
 from nova.openstack.common import log as logging
+from nova.volume import encryptors
 
 LOG = logging.getLogger(__name__)
 
@@ -209,14 +211,12 @@ class DriverVolumeBlockDevice(DriverBlockDevice):
             self['connection_info'] = None
 
     @update_db
-    def attach(self, context, instance, volume_api, virt_driver):
+    def attach(self, context, instance, volume_api, virt_driver,
+               do_check_attach=True, do_driver_attach=False):
         volume = volume_api.get(context, self.volume_id)
-        volume_api.check_attach(context, volume, instance=instance)
+        if do_check_attach:
+            volume_api.check_attach(context, volume, instance=instance)
 
-        # Attach a volume to an instance at boot time. So actual attach
-        # is done by instance creation.
-        instance_id = instance['id']
-        instance_uuid = instance['uuid']
         volume_id = volume['id']
         context = context.elevated()
 
@@ -224,12 +224,31 @@ class DriverVolumeBlockDevice(DriverBlockDevice):
         connection_info = volume_api.initialize_connection(context,
                                                            volume_id,
                                                            connector)
-        volume_api.attach(context, volume_id,
-                          instance_uuid, self['mount_device'])
-
         if 'serial' not in connection_info:
             connection_info['serial'] = self.volume_id
+
+        # If do_driver_attach is False, we will attach a volume to an instance
+        # at boot time. So actual attach is done by instance creation code.
+        if do_driver_attach:
+            encryption = encryptors.get_encryption_metadata(
+                context, volume_api, volume_id, connection_info)
+
+            try:
+                virt_driver.attach_volume(
+                        context, connection_info, instance,
+                        self['mount_device'], encryption=encryption)
+            except Exception:  # pylint: disable=W0702
+                with excutils.save_and_reraise_exception():
+                    LOG.exception(_("Driver failed to attach volume "
+                                    "%(volume_id)s at %(mountpoint)s"),
+                                  {'volume_id': volume_id,
+                                   'mountpoint': self['mount_device']},
+                                  context=context, instance=instance)
+                    volume_api.terminate_connection(context, volume_id,
+                                                    connector)
         self['connection_info'] = connection_info
+        volume_api.attach(context, volume_id,
+                          instance['uuid'], self['mount_device'])
 
     @update_db
     def refresh_connection_info(self, context, instance,