Merge "Remove check_detach"

nova/api/openstack/compute/volumes.py

@@ -411,8 +411,7 @@ class VolumeAttachmentController(wsgi.Controller):
                                                  new_volume)
                     found = True
                     break
-                except (exception.VolumeUnattached,
+                except (exception.VolumeBDMNotFound):
-                        exception.VolumeBDMNotFound):
                     # The volume is not attached.  Treat it as NotFound
                     # by falling through.
                     pass
@@ -467,10 +466,6 @@ class VolumeAttachmentController(wsgi.Controller):
                     self.compute_api.detach_volume(context, instance, volume)
                     found = True
                     break
-                except exception.VolumeUnattached:
-                    # The volume is not attached.  Treat it as NotFound
-                    # by falling through.
-                    pass
                 except exception.InvalidVolume as e:
                     raise exc.HTTPBadRequest(explanation=e.format_message())
                 except exception.InstanceUnknownCell as e:

nova/compute/api.py

@@ -3657,17 +3657,16 @@ class API(base.Base):
         return self._attach_volume(context, instance, volume_id, device,
                                    disk_bus, device_type, tag=tag)
 
-    def _check_and_begin_detach(self, context, volume, instance):
-        self.volume_api.check_detach(context, volume, instance=instance)
-        self.volume_api.begin_detaching(context, volume['id'])
-
     def _detach_volume(self, context, instance, volume):
         """Detach volume from instance.
 
         This method is separated to make it easier for cells version
         to override.
         """
-        self._check_and_begin_detach(context, volume, instance)
+        try:
+            self.volume_api.begin_detaching(context, volume['id'])
+        except exception.InvalidInput as exc:
+            raise exception.InvalidVolume(reason=exc.format_message())
         attachments = volume.get('attachments', {})
         attachment_id = None
         if attachments and instance.uuid in attachments:
@@ -3684,7 +3683,10 @@ class API(base.Base):
         If the volume has delete_on_termination option set then we call the
         volume api delete as well.
         """
-        self._check_and_begin_detach(context, volume, instance)
+        try:
+            self.volume_api.begin_detaching(context, volume['id'])
+        except exception.InvalidInput as exc:
+            raise exception.InvalidVolume(reason=exc.format_message())
         bdms = [objects.BlockDeviceMapping.get_by_volume_id(
                 context, volume['id'], instance.uuid)]
         self._local_cleanup_bdm_volumes(bdms, instance, context)
@@ -3707,8 +3709,6 @@ class API(base.Base):
                                     vm_states.RESIZED, vm_states.SOFT_DELETED])
     def swap_volume(self, context, instance, old_volume, new_volume):
         """Swap volume attached to an instance."""
-        if old_volume['attach_status'] == 'detached':
-            raise exception.VolumeUnattached(volume_id=old_volume['id'])
         # The caller likely got the instance from volume['attachments']
         # in the first place, but let's sanity check.
         if not old_volume.get('attachments', {}).get(instance.uuid):
@@ -3720,10 +3720,12 @@ class API(base.Base):
         if int(new_volume['size']) < int(old_volume['size']):
             msg = _("New volume must be the same size or larger.")
             raise exception.InvalidVolume(reason=msg)
-        self.volume_api.check_detach(context, old_volume)
         self.volume_api.check_availability_zone(context, new_volume,
                                                 instance=instance)
-        self.volume_api.begin_detaching(context, old_volume['id'])
+        try:
+            self.volume_api.begin_detaching(context, old_volume['id'])
+        except exception.InvalidInput as exc:
+            raise exception.InvalidVolume(reason=exc.format_message())
 
         # Get the BDM for the attached (old) volume so we can tell if it was
         # attached with the new-style Cinder 3.27 API.

nova/compute/cells_api.py

@@ -472,7 +472,6 @@ class ComputeCellsAPI(compute_api.API):
     @check_instance_cell
     def _detach_volume(self, context, instance, volume):
         """Detach a volume from an instance."""
-        self.volume_api.check_detach(context, volume, instance=instance)
         self._cast_to_cells(context, instance, 'detach_volume',
                 volume)
 

nova/compute/manager.py

@@ -2954,8 +2954,6 @@ class ComputeManager(manager.Manager):
                 #
                 # API-detach
                 LOG.info("Detaching from volume api: %s", volume_id)
-                volume = self.volume_api.get(context, volume_id)
-                self.volume_api.check_detach(context, volume)
                 self.volume_api.begin_detaching(context, volume_id)
 
                 # Manager-detach

nova/exception.py

@@ -254,10 +254,6 @@ class VolumeAttachFailed(Invalid):
                 "Reason: %(reason)s")
 
 
-class VolumeUnattached(Invalid):
-    msg_fmt = _("Volume %(volume_id)s is not attached to anything")
-
-
 class VolumeNotCreated(NovaException):
     msg_fmt = _("Volume %(volume_id)s did not finish being created"
                 " even after we waited %(seconds)s seconds or %(attempts)s"

nova/tests/fixtures.py

@@ -1358,10 +1358,6 @@ class CinderFixture(fixtures.Fixture):
 
         self.test.stub_out('nova.volume.cinder.API.begin_detaching',
                            lambda *args, **kwargs: None)
-        self.test.stub_out('nova.volume.cinder.API.check_attach',
-                           lambda *args, **kwargs: None)
-        self.test.stub_out('nova.volume.cinder.API.check_detach',
-                           lambda *args, **kwargs: None)
         self.test.stub_out('nova.volume.cinder.API.get',
                            fake_get)
         self.test.stub_out('nova.volume.cinder.API.initialize_connection',

nova/tests/functional/test_servers.py

@@ -994,8 +994,8 @@ class ServerTestV220(ServersTestBase):
 
         # Test detach volume
         self.stub_out('nova.volume.cinder.API.get', fakes.stub_volume_get)
-        with test.nested(mock.patch.object(compute_api.API,
+        with test.nested(mock.patch.object(volume.cinder.API,
-                                           '_check_and_begin_detach'),
+                                           'begin_detaching'),
                          mock.patch.object(objects.BlockDeviceMappingList,
                                            'get_by_instance_uuid'),
                          mock.patch.object(rpcapi.ComputeAPI,
@@ -1034,8 +1034,8 @@ class ServerTestV220(ServersTestBase):
 
         # Test detach volume
         self.stub_out('nova.volume.cinder.API.get', fakes.stub_volume_get)
-        with test.nested(mock.patch.object(compute_api.API,
+        with test.nested(mock.patch.object(volume.cinder.API,
-                                           '_check_and_begin_detach'),
+                                           'begin_detaching'),
                          mock.patch.object(objects.BlockDeviceMappingList,
                                            'get_by_instance_uuid'),
                          mock.patch.object(compute_api.API,

nova/tests/unit/compute/test_compute.py

@@ -10143,16 +10143,12 @@ class ComputeAPITestCase(BaseTestCase):
         # Set attach_status to 'fake' as nothing is reading the value.
         volume = {'id': 1, 'attach_status': 'fake'}
 
-        def fake_check_detach(*args, **kwargs):
-            called['fake_check_detach'] = True
-
         def fake_begin_detaching(*args, **kwargs):
             called['fake_begin_detaching'] = True
 
         def fake_rpc_detach_volume(self, context, **kwargs):
             called['fake_rpc_detach_volume'] = True
 
-        self.stub_out('nova.volume.cinder.API.check_detach', fake_check_detach)
         self.stub_out('nova.volume.cinder.API.begin_detaching',
                       fake_begin_detaching)
         self.stub_out('nova.compute.rpcapi.ComputeAPI.detach_volume',
@@ -10160,17 +10156,16 @@ class ComputeAPITestCase(BaseTestCase):
 
         self.compute_api.detach_volume(self.context,
                 instance, volume)
-        self.assertTrue(called.get('fake_check_detach'))
         self.assertTrue(called.get('fake_begin_detaching'))
         self.assertTrue(called.get('fake_rpc_detach_volume'))
 
-    @mock.patch.object(compute_api.API, '_check_and_begin_detach')
+    @mock.patch.object(nova.volume.cinder.API, 'begin_detaching')
     @mock.patch.object(compute_api.API, '_local_cleanup_bdm_volumes')
     @mock.patch.object(objects.BlockDeviceMapping, 'get_by_volume_id')
     def test_detach_volume_shelved_offloaded(self,
                                              mock_block_dev,
                                              mock_local_cleanup,
-                                             mock_check_begin_detach):
+                                             mock_begin_detaching):
 
         mock_block_dev.return_value = [block_device_obj.BlockDeviceMapping(
                                       context=context)]
@@ -10179,12 +10174,13 @@ class ComputeAPITestCase(BaseTestCase):
         self.compute_api._detach_volume_shelved_offloaded(self.context,
                                                           instance,
                                                           volume)
-        mock_check_begin_detach.assert_called_once_with(self.context,
+        mock_begin_detaching.assert_called_once_with(self.context,
-                                                        volume,
+                                                     volume['id'])
-                                                        instance)
         self.assertTrue(mock_local_cleanup.called)
 
-    def test_detach_invalid_volume(self):
+    @mock.patch.object(nova.volume.cinder.API, 'begin_detaching',
+                       side_effect=exception.InvalidInput(reason='error'))
+    def test_detach_invalid_volume(self, mock_begin_detaching):
         # Ensure exception is raised while detaching an un-attached volume
         fake_instance = self._fake_instance({
                     'uuid': 'f7000000-0000-0000-0000-000000000001',
@@ -10198,22 +10194,6 @@ class ComputeAPITestCase(BaseTestCase):
                           self.compute_api.detach_volume, self.context,
                           fake_instance, volume)
 
-    def test_detach_unattached_volume(self):
-        # Ensure exception is raised when volume's idea of attached
-        # instance doesn't match.
-        fake_instance = self._fake_instance({
-                    'uuid': 'f7000000-0000-0000-0000-000000000001',
-                    'locked': False,
-                    'launched_at': timeutils.utcnow(),
-                    'vm_state': vm_states.ACTIVE,
-                    'task_state': None})
-        volume = {'id': 1, 'attach_status': 'attached', 'status': 'in-use',
-                  'attachments': {'fake_uuid': {'attachment_id': 'fakeid'}}}
-
-        self.assertRaises(exception.VolumeUnattached,
-                          self.compute_api.detach_volume, self.context,
-                          fake_instance, volume)
-
     def test_detach_suspended_instance_fails(self):
         fake_instance = self._fake_instance({
                     'uuid': 'f7000000-0000-0000-0000-000000000001',

nova/tests/unit/compute/test_compute_api.py

@@ -2214,13 +2214,6 @@ class _ComputeAPIUnitTestMixIn(object):
             exception.InstanceInvalidState,
             instance_update={'vm_state': vm_states.BUILDING})
 
-    def test_swap_volume_with_old_volume_not_attached(self):
-        # Should fail if old volume is not attached
-        self._test_swap_volume_for_precheck_with_exception(
-            exception.VolumeUnattached,
-            volume_update={'target': uuids.old_volume,
-                           'value': {'attach_status': 'detached'}})
-
     def test_swap_volume_with_another_server_volume(self):
         # Should fail if old volume's instance_uuid is not that of the instance
         self._test_swap_volume_for_precheck_with_exception(

nova/tests/unit/fake_volume.py

@@ -185,18 +185,6 @@ class API(object):
                 msg = "Instance and volume not in same availability_zone"
                 raise exception.InvalidVolume(reason=msg)
 
-    def check_detach(self, context, volume, instance=None):
-        if volume['status'] == "available":
-            msg = "already detached"
-            raise exception.InvalidVolume(reason=msg)
-
-        if volume['attach_status'] == 'detached':
-            msg = "Volume must be attached in order to detach."
-            raise exception.InvalidVolume(reason=msg)
-
-        if instance and not volume.get('attachments', {}).get(instance.uuid):
-            raise exception.VolumeUnattached(volume_id=volume['id'])
-
     def attach(self, context, volume_id, instance_uuid, mountpoint, mode='rw'):
         LOG.info('attaching volume %s', volume_id)
         volume = self.get(context, volume_id)

nova/tests/unit/volume/test_cinder.py

@@ -200,29 +200,6 @@ class CinderApiTestCase(test.NoDBTestCase):
                           self.ctx, volume, instance)
         mock_get_instance_az.assert_called_once_with(self.ctx, instance)
 
-    def test_check_detach(self):
-        volume = {'id': 'fake', 'status': 'in-use',
-                  'attach_status': 'attached',
-                  'attachments': {uuids.instance: {
-                                    'attachment_id': uuids.attachment}}
-                  }
-        self.assertIsNone(self.api.check_detach(self.ctx, volume))
-        instance = fake_instance_obj(self.ctx)
-        instance.uuid = uuids.instance
-        self.assertIsNone(self.api.check_detach(self.ctx, volume, instance))
-        instance.uuid = uuids.instance2
-        self.assertRaises(exception.VolumeUnattached,
-                          self.api.check_detach, self.ctx, volume, instance)
-        volume['attachments'] = {}
-        self.assertRaises(exception.VolumeUnattached,
-                          self.api.check_detach, self.ctx, volume, instance)
-        volume['status'] = 'available'
-        self.assertRaises(exception.InvalidVolume,
-                          self.api.check_detach, self.ctx, volume)
-        volume['attach_status'] = 'detached'
-        self.assertRaises(exception.InvalidVolume,
-                          self.api.check_detach, self.ctx, volume)
-
     @mock.patch('nova.volume.cinder.cinderclient')
     def test_reserve_volume(self, mock_cinderclient):
         mock_volumes = mock.MagicMock()

nova/volume/cinder.py

@@ -337,24 +337,6 @@ class API(object):
                             'vol_zone': volume['availability_zone']}
                 raise exception.InvalidVolume(reason=msg)
 
-    def check_detach(self, context, volume, instance=None):
-        # TODO(vish): abstract status checking?
-        if volume['status'] == "available":
-            msg = _("volume %s already detached") % volume['id']
-            raise exception.InvalidVolume(reason=msg)
-
-        if volume['attach_status'] == 'detached':
-            msg = _("Volume must be attached in order to detach.")
-            raise exception.InvalidVolume(reason=msg)
-
-        # NOTE(ildikov):Preparation for multiattach support, when a volume
-        # can be attached to multiple hosts and/or instances,
-        # so just check the attachment specific to this instance
-        if instance is not None and instance.uuid not in volume['attachments']:
-            # TODO(ildikov): change it to a better exception, when enable
-            # multi-attach.
-            raise exception.VolumeUnattached(volume_id=volume['id'])
-
     @translate_volume_exception
     def reserve_volume(self, context, volume_id):
         cinderclient(context).volumes.reserve(volume_id)