Merge "Check for outstanding attachments during reserve"

cinder/tests/unit/attachments/test_attachments_api.py

@@ -178,3 +178,73 @@ class AttachmentManagerTestCase(test.TestCase):
         vref = objects.Volume.get_by_id(self.context,
                                         vref.id)
         self.assertEqual(2, len(vref.volume_attachment))
+
+    @mock.patch('cinder.volume.api.check_policy')
+    @mock.patch('cinder.volume.rpcapi.VolumeAPI.attachment_update')
+    def test_attachment_create_reserve_delete(
+            self,
+            mock_rpc_attachment_update,
+            mock_policy):
+        volume_params = {'status': 'available'}
+        connector = {
+            "initiator": "iqn.1993-08.org.debian:01:cad181614cec",
+            "ip": "192.168.1.20",
+            "platform": "x86_64",
+            "host": "tempest-1",
+            "os_type": "linux2",
+            "multipath": False}
+
+        connection_info = {'fake_key': 'fake_value',
+                           'fake_key2': ['fake_value1', 'fake_value2']}
+        mock_rpc_attachment_update.return_value = connection_info
+
+        vref = tests_utils.create_volume(self.context, **volume_params)
+        aref = self.volume_api.attachment_create(self.context,
+                                                 vref,
+                                                 fake.UUID2,
+                                                 connector=connector)
+        vref = objects.Volume.get_by_id(self.context,
+                                        vref.id)
+        # Need to set the status here because our mock isn't doing it for us
+        vref.status = 'in-use'
+        vref.save()
+
+        # Now a second attachment acting as a reserve
+        self.volume_api.attachment_create(self.context,
+                                          vref,
+                                          fake.UUID2)
+
+        # We should now be able to delete the original attachment that gave us
+        # 'in-use' status, and in turn we should revert to the outstanding
+        # attachments reserve
+        self.volume_api.attachment_delete(self.context,
+                                          aref)
+        vref = objects.Volume.get_by_id(self.context,
+                                        vref.id)
+        self.assertEqual('reserved', vref.status)
+
+    @mock.patch('cinder.volume.api.check_policy')
+    def test_reserve_reserve_delete(self, mock_policy):
+        """Test that we keep reserved status across multiple reserves."""
+        volume_params = {'status': 'available'}
+
+        vref = tests_utils.create_volume(self.context, **volume_params)
+        aref = self.volume_api.attachment_create(self.context,
+                                                 vref,
+                                                 fake.UUID2)
+        vref = objects.Volume.get_by_id(self.context,
+                                        vref.id)
+        self.assertEqual('reserved', vref.status)
+
+        self.volume_api.attachment_create(self.context,
+                                          vref,
+                                          fake.UUID2)
+        vref = objects.Volume.get_by_id(self.context,
+                                        vref.id)
+        self.assertEqual('reserved', vref.status)
+        self.volume_api.attachment_delete(self.context,
+                                          aref)
+        vref = objects.Volume.get_by_id(self.context,
+                                        vref.id)
+        self.assertEqual('reserved', vref.status)
+        self.assertEqual(1, len(vref.volume_attachment))

cinder/volume/api.py

@@ -2043,19 +2043,37 @@ class API(base.Base):
             self.volume_rpcapi.attachment_delete(ctxt,
                                                  attachment.id,
                                                  volume)
+        status_updates = {'status': 'available',
+                          'attach_status': 'detached'}
         remaining_attachments = AO_LIST.get_all_by_volume_id(ctxt, volume.id)
 
-        # TODO(jdg): Make this check attachments_by_volume_id when we
-        # implement multi-attach for real
-        if len(remaining_attachments) < 1:
-            volume.status = 'available'
-            volume.attach_status = 'detached'
-            volume.save()
+        # NOTE(jdg) Try and figure out the > state we have left and set that
+        # attached > attaching > > detaching > reserved
+        pending_status_list = []
+        for attachment in remaining_attachments:
+            pending_status_list.append(attachment.attach_status)
+        if 'attached' in pending_status_list:
+            status_updates['status'] = 'in-use'
+            status_updates['attach_status'] = 'attached'
+        elif 'attaching' in pending_status_list:
+            status_updates['status'] = 'attaching'
+            status_updates['attach_status'] = 'attaching'
+        elif 'detaching' in pending_status_list:
+            status_updates['status'] = 'detaching'
+            status_updates['attach_status'] = 'detaching'
+        elif 'reserved' in pending_status_list:
+            status_updates['status'] = 'reserved'
+            status_updates['attach_status'] = 'reserved'
+
+        volume.status = status_updates['status']
+        volume.attach_status = status_updates['attach_status']
+        volume.save()
         return remaining_attachments
 
 
 class HostAPI(base.Base):
     """Sub-set of the Volume Manager API for managing host operations."""
+
     def set_host_enabled(self, context, host, enabled):
         """Sets the specified host's ability to accept new volumes."""
         raise NotImplementedError()