Update detach_volume() with versionedobjects

The following patch updates detach_volume
API to use volume versionedobjects.  Changes were
made to be backwards compatible with older RPC clients.
It only includes changes to the core cinder code.
Changes in the drivers are left to each driver
maintainer to update.

We want to update both the attach_volume()
and detach_volume() with versionedobjects.
There are 3 patches in the chain.

The patch sequence is:
- Update attach_volume() with versionedojbects
- Update detach_volume() with versionedobjects
- Update test cases related to attach_volume
  and detach_volume in file
  cinder/tests/unit/test_volume.py

Co-Authored-By: Thang Pham <thang.g.pham@gmail.com>
Co-Authored-By: lisali <xiaoyan.li@intel.com>
Partial-Implements: blueprint cinder-objects

Change-Id: Ib153342b20f5b3e74d2815cb422b411f5690443f

cinder/db/sqlalchemy/api.py

@@ -1608,44 +1608,50 @@ def volume_detached(context, volume_id, attachment_id):
     """
     session = get_session()
     with session.begin():
-        attachment = None
         try:
             attachment = volume_attachment_get(context, attachment_id,
                                                session=session)
         except exception.VolumeAttachmentNotFound:
-            pass
+            attachment_updates = None
+            attachment = None
 
-        # If this is already detached, attachment will be None
         if attachment:
             now = timeutils.utcnow()
-            attachment['attach_status'] = fields.VolumeAttachStatus.DETACHED
+            attachment_updates = {
-            attachment['detach_time'] = now
+                'attach_status': fields.VolumeAttachStatus.DETACHED,
-            attachment['deleted'] = True
+                'detach_time': now,
-            attachment['deleted_at'] = now
+                'deleted': True,
+                'deleted_at': now,
+                'updated_at':
+                literal_column('updated_at'),
+            }
+            attachment.update(attachment_updates)
             attachment.save(session=session)
+            del attachment_updates['updated_at']
 
-        attachment_list = volume_attachment_get_all_by_volume_id(
+        volume_ref = _volume_get(context, volume_id,
-            context, volume_id, session=session)
+                                 session=session)
-        remain_attachment = False
+        volume_updates = {'updated_at': literal_column('updated_at')}
-        if attachment_list and len(attachment_list) > 0:
+        if not volume_ref.volume_attachment:
-            remain_attachment = True
-
-        volume_ref = _volume_get(context, volume_id, session=session)
-        if not remain_attachment:
             # Hide status update from user if we're performing volume migration
             # or uploading it to image
-            if ((not volume_ref['migration_status'] and
+            if ((not volume_ref.migration_status and
-                    not (volume_ref['status'] == 'uploading')) or
+                    not (volume_ref.status == 'uploading')) or
-                    volume_ref['migration_status'] in ('success', 'error')):
+                    volume_ref.migration_status in ('success', 'error')):
-                volume_ref['status'] = 'available'
+                volume_updates['status'] = 'available'
 
-            volume_ref['attach_status'] = fields.VolumeAttachStatus.DETACHED
+            volume_updates['attach_status'] = (
-            volume_ref.save(session=session)
+                fields.VolumeAttachStatus.DETACHED)
         else:
             # Volume is still attached
-            volume_ref['status'] = 'in-use'
+            volume_updates['status'] = 'in-use'
-            volume_ref['attach_status'] = fields.VolumeAttachStatus.ATTACHED
+            volume_updates['attach_status'] = (
+                fields.VolumeAttachStatus.ATTACHED)
+
+        volume_ref.update(volume_updates)
         volume_ref.save(session=session)
+        del volume_updates['updated_at']
+        return (volume_updates, attachment_updates)
 
 
 @require_context

cinder/objects/volume.py

@@ -517,6 +517,25 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject,
             self.save()
         return attachment
 
+    def finish_detach(self, attachment_id):
+        with self.obj_as_admin():
+            volume_updates, attachment_updates = (
+                db.volume_detached(self._context, self.id, attachment_id))
+            db.volume_admin_metadata_delete(self._context, self.id,
+                                            'attached_mode')
+            self.admin_metadata.pop('attached_mode', None)
+        # Remove attachment in volume only when this field is loaded.
+        if attachment_updates and self.obj_attr_is_set('volume_attachment'):
+            for i, attachment in enumerate(self.volume_attachment):
+                if attachment.id == attachment_id:
+                    del self.volume_attachment.objects[i]
+                    break
+
+        self.update(volume_updates)
+        self.obj_reset_changes(
+            list(volume_updates.keys()) +
+            ['volume_attachment', 'admin_metadata'])
+
 
 @base.CinderObjectRegistry.register
 class VolumeList(base.ObjectListBase, base.CinderObject):

cinder/tests/unit/objects/test_volume.py

@@ -435,6 +435,73 @@ class TestVolume(test_objects.BaseObjectsTestCase):
                                                     True)
             self.assertEqual('rw', volume.admin_metadata['attached_mode'])
 
+    @mock.patch('cinder.db.volume_admin_metadata_delete')
+    @mock.patch('cinder.db.sqlalchemy.api.volume_detached')
+    @mock.patch('cinder.objects.volume_attachment.VolumeAttachmentList.'
+                'get_all_by_volume_id')
+    def test_volume_detached_with_attachment(
+            self, volume_attachment_get,
+            volume_detached,
+            metadata_delete):
+        va_objs = [objects.VolumeAttachment(context=self.context, id=i)
+                   for i in [fake.OBJECT_ID, fake.OBJECT2_ID, fake.OBJECT3_ID]]
+        # As changes are not saved, we need reset it here. Later changes
+        # will be checked.
+        for obj in va_objs:
+            obj.obj_reset_changes()
+        va_list = objects.VolumeAttachmentList(context=self.context,
+                                               objects=va_objs)
+        va_list.obj_reset_changes()
+        volume_attachment_get.return_value = va_list
+        admin_context = context.get_admin_context()
+        volume = fake_volume.fake_volume_obj(
+            admin_context,
+            volume_attachment=va_list,
+            volume_admin_metadata=[{'key': 'attached_mode',
+                                    'value': 'rw'}])
+        self.assertEqual(3, len(volume.volume_attachment))
+        volume_detached.return_value = ({'status': 'in-use'},
+                                        {'attached_mode': 'rw'})
+        with mock.patch.object(admin_context, 'elevated') as mock_elevated:
+            mock_elevated.return_value = admin_context
+            volume.finish_detach(fake.OBJECT_ID)
+            volume_detached.assert_called_once_with(admin_context,
+                                                    volume.id,
+                                                    fake.OBJECT_ID)
+            metadata_delete.assert_called_once_with(admin_context,
+                                                    volume.id,
+                                                    'attached_mode')
+            self.assertEqual('in-use', volume.status)
+            self.assertEqual({}, volume.cinder_obj_get_changes())
+            self.assertEqual(2, len(volume.volume_attachment))
+            self.assertIsNone(volume.admin_metadata.get('attached_mode'))
+
+    @mock.patch('cinder.db.volume_admin_metadata_delete')
+    @mock.patch('cinder.db.sqlalchemy.api.volume_detached')
+    @mock.patch('cinder.objects.volume_attachment.VolumeAttachmentList.'
+                'get_all_by_volume_id')
+    def test_volume_detached_without_attachment(
+            self, volume_attachment_get, volume_detached, metadata_delete):
+        admin_context = context.get_admin_context()
+        volume = fake_volume.fake_volume_obj(
+            admin_context,
+            volume_admin_metadata=[{'key': 'attached_mode',
+                                    'value': 'rw'}])
+        self.assertFalse(volume.obj_attr_is_set('volume_attachment'))
+        volume_detached.return_value = ({'status': 'in-use'}, None)
+        with mock.patch.object(admin_context, 'elevated') as mock_elevated:
+            mock_elevated.return_value = admin_context
+            volume.finish_detach(fake.OBJECT_ID)
+            metadata_delete.assert_called_once_with(admin_context,
+                                                    volume.id,
+                                                    'attached_mode')
+            volume_detached.assert_called_once_with(admin_context,
+                                                    volume.id,
+                                                    fake.OBJECT_ID)
+            self.assertEqual('in-use', volume.status)
+            self.assertEqual({}, volume.cinder_obj_get_changes())
+            self.assertFalse(volume_attachment_get.called)
+
 
 class TestVolumeList(test_objects.BaseObjectsTestCase):
     @mock.patch('cinder.db.volume_get_all')

cinder/tests/unit/test_db_api.py

@@ -387,33 +387,101 @@ class DBAPIVolumeTestCase(BaseTest):
                   'instance_uuid': instance_uuid,
                   'attach_status': fields.VolumeAttachStatus.ATTACHING, }
         attachment = db.volume_attach(self.ctxt, values)
-        db.volume_attached(self.ctxt, attachment['id'],
+        db.volume_attached(self.ctxt, attachment.id,
                            instance_uuid,
                            None, '/tmp')
-        db.volume_detached(self.ctxt, volume['id'], attachment['id'])
+        volume_updates, attachment_updates = (
-        volume = db.volume_get(self.ctxt, volume['id'])
+            db.volume_detached(self.ctxt, volume.id, attachment.id))
+        expected_attachment = {
+            'attach_status': fields.VolumeAttachStatus.DETACHED,
+            'detach_time': mock.ANY,
+            'deleted': True,
+            'deleted_at': mock.ANY, }
+        self.assertDictEqual(expected_attachment, attachment_updates)
+        expected_volume = {
+            'status': 'available',
+            'attach_status': fields.VolumeAttachStatus.DETACHED, }
+        self.assertDictEqual(expected_volume, volume_updates)
+        volume = db.volume_get(self.ctxt, volume.id)
         self.assertRaises(exception.VolumeAttachmentNotFound,
                           db.volume_attachment_get,
                           self.ctxt,
-                          attachment['id'])
+                          attachment.id)
-        self.assertEqual('available', volume['status'])
+        self.assertEqual('available', volume.status)
+
+    def test_volume_detached_two_attachments(self):
+        volume = db.volume_create(self.ctxt, {})
+        instance_uuid = fake.INSTANCE_ID
+        values = {'volume_id': volume.id,
+                  'instance_uuid': instance_uuid,
+                  'attach_status': fields.VolumeAttachStatus.ATTACHING, }
+        attachment = db.volume_attach(self.ctxt, values)
+        values2 = {'volume_id': volume.id,
+                   'instance_uuid': fake.OBJECT_ID,
+                   'attach_status': fields.VolumeAttachStatus.ATTACHING, }
+        db.volume_attach(self.ctxt, values2)
+        db.volume_attached(self.ctxt, attachment.id,
+                           instance_uuid,
+                           None, '/tmp')
+        volume_updates, attachment_updates = (
+            db.volume_detached(self.ctxt, volume.id, attachment.id))
+        expected_attachment = {
+            'attach_status': fields.VolumeAttachStatus.DETACHED,
+            'detach_time': mock.ANY,
+            'deleted': True,
+            'deleted_at': mock.ANY, }
+        self.assertDictEqual(expected_attachment, attachment_updates)
+        expected_volume = {
+            'status': 'in-use',
+            'attach_status': fields.VolumeAttachStatus.ATTACHED, }
+        self.assertDictEqual(expected_volume, volume_updates)
+        volume = db.volume_get(self.ctxt, volume.id)
+        self.assertRaises(exception.VolumeAttachmentNotFound,
+                          db.volume_attachment_get,
+                          self.ctxt,
+                          attachment.id)
+        self.assertEqual('in-use', volume.status)
+
+    def test_volume_detached_invalid_attachment(self):
+        volume = db.volume_create(self.ctxt, {})
+        # detach it again
+        volume_updates, attachment_updates = (
+            db.volume_detached(self.ctxt, volume.id, fake.ATTACHMENT_ID))
+        self.assertIsNone(attachment_updates)
+        expected_volume = {
+            'status': 'available',
+            'attach_status': fields.VolumeAttachStatus.DETACHED, }
+        self.assertDictEqual(expected_volume, volume_updates)
+        volume = db.volume_get(self.ctxt, volume.id)
+        self.assertEqual('available', volume.status)
 
     def test_volume_detached_from_host(self):
         volume = db.volume_create(self.ctxt, {})
         host_name = 'fake_host'
-        values = {'volume_id': volume['id'],
+        values = {'volume_id': volume.id,
                   'attach_host': host_name,
                   'attach_status': fields.VolumeAttachStatus.ATTACHING, }
         attachment = db.volume_attach(self.ctxt, values)
-        db.volume_attached(self.ctxt, attachment['id'],
+        db.volume_attached(self.ctxt, attachment.id,
                            None, host_name, '/tmp')
-        db.volume_detached(self.ctxt, volume['id'], attachment['id'])
+        volume_updates, attachment_updates = (
-        volume = db.volume_get(self.ctxt, volume['id'])
+            db.volume_detached(self.ctxt, volume.id, attachment.id))
+        expected_attachment = {
+            'attach_status': fields.VolumeAttachStatus.DETACHED,
+            'detach_time': mock.ANY,
+            'deleted': True,
+            'deleted_at': mock.ANY}
+        self.assertDictEqual(expected_attachment, attachment_updates)
+        expected_volume = {
+            'status': 'available',
+            'attach_status': fields.VolumeAttachStatus.DETACHED, }
+        self.assertDictEqual(expected_volume, volume_updates)
+        volume = db.volume_get(self.ctxt, volume.id)
         self.assertRaises(exception.VolumeAttachmentNotFound,
                           db.volume_attachment_get,
                           self.ctxt,
-                          attachment['id'])
+                          attachment.id)
-        self.assertEqual('available', volume['status'])
+        self.assertEqual('available', volume.status)
 
     def test_volume_get(self):
         volume = db.volume_create(self.ctxt, {})

cinder/tests/unit/test_volume.py

@@ -2087,7 +2087,7 @@ class VolumeTestCase(base.BaseVolumeTestCase):
         with volume.obj_as_admin():
             volume.admin_metadata['readonly'] = True
             volume.save()
-        volume_id = volume['id']
+        volume_id = volume.id
         self.volume.create_volume(self.user_context,
                                   volume=volume)
         volume_passed = volume if volume_object else None
@@ -2120,10 +2120,12 @@ class VolumeTestCase(base.BaseVolumeTestCase):
         self.assertRaises(exception.VolumeAttached,
                           self.volume.delete_volume,
                           self.context,
-                          volume)
+                          volume=volume)
-        self.volume.detach_volume(self.context, volume_id, attachment['id'])
+        self.volume.detach_volume(self.context, volume_id,
-        vol = db.volume_get(self.context, volume_id)
+                                  attachment.id,
-        self.assertEqual('available', vol['status'])
+                                  volume=volume_passed)
+        vol = objects.Volume.get_by_id(self.context, volume_id)
+        self.assertEqual('available', vol.status)
 
         self.volume.delete_volume(self.context, volume)
         self.assertRaises(exception.VolumeNotFound,

cinder/tests/unit/test_volume_rpcapi.py

@@ -439,20 +439,26 @@ class VolumeRpcAPITestCase(test.TestCase):
                               mode='rw',
                               version=version)
 
-    def test_detach_volume(self):
+    @ddt.data('3.0', '3.4')
+    @mock.patch('oslo_messaging.RPCClient.can_send_version')
+    def test_detach_volume(self, version, can_send_version):
+        can_send_version.return_value = (version == '3.4')
         self._test_volume_api('detach_volume',
                               rpc_method='call',
                               volume=self.fake_volume_obj,
                               attachment_id='fake_uuid',
-                              version="3.0")
+                              version=version)
 
-    def test_detach_volume_cluster(self):
+    @ddt.data('3.0', '3.4')
+    @mock.patch('oslo_messaging.RPCClient.can_send_version')
+    def test_detach_volume_cluster(self, version, can_send_version):
+        can_send_version.return_value = (version == '3.4')
         self._set_cluster()
         self._test_volume_api('detach_volume',
                               rpc_method='call',
                               volume=self.fake_volume_obj,
                               attachment_id='fake_uuid',
-                              version="3.0")
+                              version=version)
 
     def test_copy_volume_to_image(self):
         self._test_volume_api('copy_volume_to_image',

cinder/volume/manager.py

@@ -1022,14 +1022,19 @@ class VolumeManager(manager.CleanableManager,
         return attachment
 
     @coordination.synchronized('{volume_id}-{f_name}')
-    def detach_volume(self, context, volume_id, attachment_id=None):
+    def detach_volume(self, context, volume_id, attachment_id=None,
+                      volume=None):
         """Updates db to show volume is detached."""
         # TODO(vish): refactor this into a more general "unreserve"
-        volume = self.db.volume_get(context, volume_id)
+        # FIXME(lixiaoy1): Remove this in v4.0 of RPC API.
-        attachment = None
+        if volume is None:
+            # For older clients, mimic the old behavior and look up the volume
+            # by its volume_id.
+            volume = objects.Volume.get_by_id(context, volume_id)
+
         if attachment_id:
             try:
-                attachment = self.db.volume_attachment_get(context,
+                attachment = objects.VolumeAttachment.get_by_id(context,
                                                                 attachment_id)
             except exception.VolumeAttachmentNotFound:
                 LOG.info(_LI("Volume detach called, but volume not attached."),
@@ -1037,14 +1042,13 @@ class VolumeManager(manager.CleanableManager,
                 # We need to make sure the volume status is set to the correct
                 # status.  It could be in detaching status now, and we don't
                 # want to leave it there.
-                self.db.volume_detached(context, volume_id, attachment_id)
+                volume.finish_detach(attachment_id)
                 return
         else:
             # We can try and degrade gracefully here by trying to detach
             # a volume without the attachment_id here if the volume only has
             # one attachment.  This is for backwards compatibility.
-            attachments = self.db.volume_attachment_get_all_by_volume_id(
+            attachments = volume.volume_attachment
-                context, volume_id)
             if len(attachments) > 1:
                 # There are more than 1 attachments for this volume
                 # we have to have an attachment id.
@@ -1059,10 +1063,9 @@ class VolumeManager(manager.CleanableManager,
                 # so set the status to available and move on.
                 LOG.info(_LI("Volume detach called, but volume not attached."),
                          resource=volume)
-                self.db.volume_update(
+                volume.status = 'available'
-                    context, volume_id, {
+                volume.attach_status = fields.VolumeAttachStatus.DETACHED
-                        'status': 'available',
+                volume.save()
-                        'attach_status': fields.VolumeAttachStatus.DETACHED})
                 return
 
         self._notify_about_volume_usage(context, volume, "detach.start")
@@ -1092,7 +1095,6 @@ class VolumeManager(manager.CleanableManager,
 
         # We're going to remove the export here
         # (delete the iscsi target)
-        volume = self.db.volume_get(context, volume_id)
         try:
             utils.require_driver_initialized(self.driver)
             self.driver.remove_export(context.elevated(), volume)
@@ -1108,11 +1110,7 @@ class VolumeManager(manager.CleanableManager,
             raise exception.RemoveExportException(volume=volume_id,
                                                   reason=six.text_type(ex))
 
-        self.db.volume_detached(context.elevated(), volume_id,
+        volume.finish_detach(attachment.id)
-                                attachment.get('id'))
-        self.db.volume_admin_metadata_delete(context.elevated(), volume_id,
-                                             'attached_mode')
-
         self._notify_about_volume_usage(context, volume, "detach.end")
         LOG.info(_LI("Detach volume completed successfully."), resource=volume)
 

cinder/volume/rpcapi.py

@@ -114,9 +114,10 @@ class VolumeAPI(rpc.RPCAPI):
         3.2  - Adds support for sending objects over RPC in
                get_backup_device().
         3.3  - Adds support for sending objects over RPC in attach_volume().
+        3.4  - Adds support for sending objects over RPC in detach_volume().
     """
 
-    RPC_API_VERSION = '3.3'
+    RPC_API_VERSION = '3.4'
     RPC_DEFAULT_VERSION = '3.0'
     TOPIC = constants.VOLUME_TOPIC
     BINARY = 'cinder-volume'
@@ -201,9 +202,13 @@ class VolumeAPI(rpc.RPCAPI):
         return cctxt.call(ctxt, 'attach_volume', **msg_args)
 
     def detach_volume(self, ctxt, volume, attachment_id):
-        cctxt = self._get_cctxt(volume.service_topic_queue)
+        msg_args = {'volume_id': volume.id,
-        return cctxt.call(ctxt, 'detach_volume', volume_id=volume.id,
+                    'attachment_id': attachment_id,
-                          attachment_id=attachment_id)
+                    'volume': volume}
+        cctxt = self._get_cctxt(volume.service_topic_queue, ('3.4', '3.0'))
+        if not self.client.can_send_version('3.4'):
+            msg_args.pop('volume')
+        return cctxt.call(ctxt, 'detach_volume', **msg_args)
 
     def copy_volume_to_image(self, ctxt, volume, image_meta):
         cctxt = self._get_cctxt(volume['host'])