Merge "Delete attachment on remove_export failure" into stable/wallaby

3 files changed, 35 insertions, 10 deletions

diff --git a/cinder/tests/unit/attachments/test_attachments_manager.py b/cinder/tests/unit/attachments/test_attachments_manager.py
index b5799765b..0542ff73c 100644
--- a/cinder/tests/unit/attachments/test_attachments_manager.py
+++ b/cinder/tests/unit/attachments/test_attachments_manager.py
@@ -144,6 +144,31 @@ class AttachmentManagerTestCase(test.TestCase):
                           self.context,
                           attachment_ref.id)
 
+    def test_attachment_delete_remove_export_fail(self):
+        """attachment_delete removes attachment on remove_export failure."""
+        self.mock_object(self.manager.driver, 'remove_export',
+                         side_effect=Exception)
+        # Report that the connection is not shared
+        self.mock_object(self.manager, '_connection_terminate',
+                         return_value=False)
+
+        vref = tests_utils.create_volume(self.context, status='in-use',
+                                         attach_status='attached')
+        values = {'volume_id': vref.id, 'volume_host': vref.host,
+                  'attach_status': 'reserved', 'instance_uuid': fake.UUID1}
+        attach = db.volume_attach(self.context, values)
+        # Confirm the volume OVO has the attachment before the deletion
+        vref.refresh()
+        self.assertEqual(1, len(vref.volume_attachment))
+
+        self.manager.attachment_delete(self.context, attach.id, vref)
+
+        # Attachment has been removed from the DB
+        self.assertRaises(exception.VolumeAttachmentNotFound,
+                          db.volume_attachment_get, self.context, attach.id)
+        # Attachment has been removed from the volume OVO attachment list
+        self.assertEqual(0, len(vref.volume_attachment))
+
     def test_attachment_delete_multiple_attachments(self):
         volume_params = {'status': 'available'}
         vref = tests_utils.create_volume(self.context, **volume_params)
diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index 00562c86c..6613aecf3 100644
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -4846,16 +4846,10 @@ class VolumeManager(manager.CleanableManager,
             if has_shared_connection is not None and not has_shared_connection:
                 self.driver.remove_export(context.elevated(), vref)
         except Exception:
-            # FIXME(jdg): Obviously our volume object is going to need some
-            # changes to deal with multi-attach and figuring out how to
-            # represent a single failed attach out of multiple attachments
-
-            # TODO(jdg): object method here
-            attachment.attach_status = \
-                fields.VolumeAttachStatus.ERROR_DETACHING
-            attachment.save()
-        else:
-            vref.finish_detach(attachment.id)
+            # Failures on detach_volume and remove_export are not considered
+            # failures in terms of detaching the volume.
+            pass
+        vref.finish_detach(attachment.id)
         self._notify_about_volume_usage(context, vref, "detach.end")
 
     # Replication group API (Tiramisu)
diff --git a/releasenotes/notes/remove_export_failure_leaves_attachment-24e0c648269b0177.yaml b/releasenotes/notes/remove_export_failure_leaves_attachment-24e0c648269b0177.yaml
new file mode 100644
index 000000000..cde05ed17
--- /dev/null
+++ b/releasenotes/notes/remove_export_failure_leaves_attachment-24e0c648269b0177.yaml
@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    `Bug #1935057 <https://bugs.launchpad.net/cinder/+bug/1935057>`_: Fixed
+    sometimes on a detach volume may end in available and detached yet have an
+    attachment in error_detaching.