From 68d49445778ef486e3ff656929405ab270a5a65d Mon Sep 17 00:00:00 2001
From: Gorka Eguileor <geguileo@redhat.com>
Date: Fri, 2 Jul 2021 15:11:53 +0200
Subject: [PATCH] Fix detach notification

Our current `attachment_delete` methods in the volume API and the
manager are using DB methods directly, which makes the OVOs present in
those methods get out of sync with the latest data, which leads to
notifications having the wrong data when we send them on volume detach.

This patch replaces DB method calls with OVO calls and moves the
notification call to the end of the method, where we have the final
status on the volume.

It also adds the missing detach.start notification when deleting an
attachment in the reserved state.

Closes-Bug: #1916980
Closes-Bug: #1935011
Change-Id: Ie48cf55deacd08e7716201dac00ede8d57e6632f
---
 .../unit/attachments/test_attachments_manager.py   |  6 ++++++
 cinder/volume/api.py                               | 14 ++++++++------
 cinder/volume/manager.py                           | 12 ++++--------
 .../detach-notification-31ae15dafdef36c1.yaml      |  9 +++++++++
 4 files changed, 27 insertions(+), 14 deletions(-)
 create mode 100644 releasenotes/notes/detach-notification-31ae15dafdef36c1.yaml

diff --git a/cinder/tests/unit/attachments/test_attachments_manager.py b/cinder/tests/unit/attachments/test_attachments_manager.py
index 214d5690fab..bc3429b6a25 100644
--- a/cinder/tests/unit/attachments/test_attachments_manager.py
+++ b/cinder/tests/unit/attachments/test_attachments_manager.py
@@ -166,6 +166,12 @@ class AttachmentManagerTestCase(test.TestCase):
                   mock_db_detached, mock_db_meta_delete, mock_get_attachment):
             mock_elevated.return_value = self.context
             mock_con_term.return_value = False
+            mock_db_detached.return_value = (
+                {'status': 'available',
+                 'attach_status': fields.VolumeAttachStatus.DETACHED},
+                {'attach_status': fields.VolumeAttachStatus.DETACHED,
+                 'deleted': True}
+            )
 
             # test single attachment. This should call
             # detach and remove_export
diff --git a/cinder/volume/api.py b/cinder/volume/api.py
index 8482c4d210f..8b4eaac5836 100644
--- a/cinder/volume/api.py
+++ b/cinder/volume/api.py
@@ -2259,17 +2259,17 @@ class API(base.Base):
         ctxt.authorize(attachment_policy.DELETE_POLICY,
                        target_obj=attachment)
         volume = attachment.volume
+
         if attachment.attach_status == fields.VolumeAttachStatus.RESERVED:
-            self.db.volume_detached(ctxt.elevated(), attachment.volume_id,
-                                    attachment.get('id'))
-            self.db.volume_admin_metadata_delete(ctxt.elevated(),
-                                                 attachment.volume_id,
-                                                 'attached_mode')
-            volume_utils.notify_about_volume_usage(ctxt, volume, "detach.end")
+            volume_utils.notify_about_volume_usage(ctxt, volume,
+                                                   "detach.start")
+            volume.finish_detach(attachment.id)
+            do_notify = True
         else:
             self.volume_rpcapi.attachment_delete(ctxt,
                                                  attachment.id,
                                                  volume)
+            do_notify = False
         status_updates = {'status': 'available',
                           'attach_status': 'detached'}
         remaining_attachments = AO_LIST.get_all_by_volume_id(ctxt, volume.id)
@@ -2305,6 +2305,8 @@ class API(base.Base):
         volume.attach_status = status_updates['attach_status']
         volume.save()
 
+        if do_notify:
+            volume_utils.notify_about_volume_usage(ctxt, volume, "detach.end")
         return remaining_attachments
 
 
diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index 2c5cf065b53..401fbd639e0 100644
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -4954,15 +4954,11 @@ class VolumeManager(manager.CleanableManager,
             # represent a single failed attach out of multiple attachments
 
             # TODO(jdg): object method here
-            self.db.volume_attachment_update(
-                context, attachment.get('id'),
-                {'attach_status': fields.VolumeAttachStatus.ERROR_DETACHING})
+            attachment.attach_status = \
+                fields.VolumeAttachStatus.ERROR_DETACHING
+            attachment.save()
         else:
-            self.db.volume_detached(context.elevated(), vref.id,
-                                    attachment.get('id'))
-            self.db.volume_admin_metadata_delete(context.elevated(),
-                                                 vref.id,
-                                                 'attached_mode')
+            vref.finish_detach(attachment_id)
         self._notify_about_volume_usage(context, vref, "detach.end")
 
     # Replication group API (Tiramisu)
diff --git a/releasenotes/notes/detach-notification-31ae15dafdef36c1.yaml b/releasenotes/notes/detach-notification-31ae15dafdef36c1.yaml
new file mode 100644
index 00000000000..271b66588bd
--- /dev/null
+++ b/releasenotes/notes/detach-notification-31ae15dafdef36c1.yaml
@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    `Bug #1916980 <https://bugs.launchpad.net/cinder/+bug/1916980>`_: Fixed
+    stale volume notification information on volume detach.
+  - |
+    `Bug #1935011 <https://bugs.launchpad.net/cinder/+bug/1935011>`_: Fixed
+    missing detach.start notification when deleting an attachment in reserved
+    state.