From: Walter A. Boring IV <walter.boring@hp.com>
Date: Wed, 12 Aug 2015 21:00:54 +0000 (-0700)
Subject: Don't return Exception when volume is detached
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=3b4e8c31a2c94cac3e1a5c071eb38c7d5cb9a239;p=openstack-build%2Fcinder-build.git

Don't return Exception when volume is detached

This patch changes the way we handle volume detach
attempts when the attachment_id is already detached
and/or when the volume has no attachments.  We now
handle this the same way we do with deleting volumes
that don't exist.  We return success.

This patch also takes care to make sure we safely reset
the volume status to what it should be depending on if
there are other attachments.  If the attachment_id is
passed in and that attachment is detached, but there are
other attachments, we want to make sure that the volume is
left in an in-use state, not available.

Change-Id: I11b7c45bb6570ce11e13e487cf1136ca2551036b
Closes-Bug: #1484194
Related-Bug: #1476806
---

diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py
index da6f14940..fd1bfb54f 100644
--- a/cinder/db/sqlalchemy/api.py
+++ b/cinder/db/sqlalchemy/api.py
@@ -1216,8 +1216,12 @@ def volume_detached(context, volume_id, attachment_id):
     """
     session = get_session()
     with session.begin():
-        attachment = volume_attachment_get(context, attachment_id,
-                                           session=session)
+        attachment = None
+        try:
+            attachment = volume_attachment_get(context, attachment_id,
+                                               session=session)
+        except exception.VolumeAttachmentNotFound:
+            pass
 
         # If this is already detached, attachment will be None
         if attachment:
diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py
index 4a66f1528..761b385a1 100644
--- a/cinder/tests/unit/test_volume.py
+++ b/cinder/tests/unit/test_volume.py
@@ -2117,26 +2117,35 @@ class VolumeTestCase(BaseVolumeTestCase):
     def test_detach_invalid_attachment_id(self):
         """Make sure if the attachment id isn't found we raise."""
         attachment_id = "notfoundid"
-        volume_id = "abc123"
-        fake_volume = {'id': volume_id,
-                       'status': 'available'}
-        with mock.patch.object(db, 'volume_get') as mock_volume_get:
-            mock_volume_get.return_value = fake_volume
-            self.assertRaises(exception.VolumeAttachmentNotFound,
-                              self.volume.detach_volume,
-                              self.context,
-                              volume_id,
-                              attachment_id)
+        volume = tests_utils.create_volume(self.context,
+                                           admin_metadata={'readonly': 'True'},
+                                           multiattach=False,
+                                           **self.volume_params)
+        self.volume.detach_volume(self.context, volume['id'],
+                                  attachment_id)
+        volume = db.volume_get(self.context, volume['id'])
+        self.assertEqual('available', volume['status'])
+
+        instance_uuid = '12345678-1234-5678-1234-567812345678'
+        attached_host = 'fake_host'
+        mountpoint = '/dev/fake'
+        tests_utils.attach_volume(self.context, volume['id'],
+                                  instance_uuid, attached_host,
+                                  mountpoint)
+        self.volume.detach_volume(self.context, volume['id'],
+                                  attachment_id)
+        volume = db.volume_get(self.context, volume['id'])
+        self.assertEqual('in-use', volume['status'])
 
     def test_detach_no_attachments(self):
+        self.volume_params['status'] = 'detaching'
         volume = tests_utils.create_volume(self.context,
                                            admin_metadata={'readonly': 'True'},
                                            multiattach=False,
                                            **self.volume_params)
-        self.assertRaises(exception.InvalidVolume,
-                          self.volume.detach_volume,
-                          self.context,
-                          volume['id'])
+        self.volume.detach_volume(self.context, volume['id'])
+        volume = db.volume_get(self.context, volume['id'])
+        self.assertEqual('available', volume['status'])
 
     def test_run_attach_detach_volume_for_instance_no_attachment_id(self):
         """Make sure volume can be attached and detached from instance."""
diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index 534df2c58..00c40264f 100644
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -863,9 +863,13 @@ class VolumeManager(manager.SchedulerDependentManager):
                 attachment = self.db.volume_attachment_get(context,
                                                            attachment_id)
             except exception.VolumeAttachmentNotFound:
-                LOG.error(_LE("Find attachment in detach_volume failed."),
-                          resource=volume)
-                raise
+                LOG.info(_LI("Volume detach called, but volume not attached."),
+                         resource=volume)
+                # 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)
+                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
@@ -883,10 +887,13 @@ class VolumeManager(manager.SchedulerDependentManager):
                 attachment = attachments[0]
             else:
                 # there aren't any attachments for this volume.
-                msg = _("Detach volume failed, because there are currently no "
-                        "active attachments.")
-                LOG.error(msg, resource=volume)
-                raise exception.InvalidVolume(reason=msg)
+                # 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(context, volume_id,
+                                      {'status': 'available',
+                                       'attach_status': 'detached'})
+                return
 
         self._notify_about_volume_usage(context, volume, "detach.start")
         try: