From 9db3eba8133f4f643bc4a748460a97434fa91f78 Mon Sep 17 00:00:00 2001
From: Sean Mooney <work@seanmooney.info>
Date: Mon, 21 Oct 2024 20:50:20 +0100
Subject: [PATCH] Fix detaching devices by alias with mdevs

This change fixes the get_device_by_alias function
to properly handle devices that do not have an
alias in the domain xml.

By definition if a device does not have an alias
it can't match a given alias, so this change adds
a hasattr check to ensure we do not attempt to check
the alias if it's not defined.

This eliminates this entire class of bugs for all
devices instead of just fixing it for a specific class.

Related-Bug: #1942345
Closes-Bug: #2074219
Change-Id: If417a43ea252647618e50391b63333f6b68bdfec
---
 .../functional/regressions/test_bug_2074219.py  | 13 +++----------
 nova/tests/unit/virt/libvirt/test_guest.py      |  5 +++++
 nova/virt/libvirt/guest.py                      |  2 +-
 .../notes/bug-2074219-937d6404c1cbb04c.yaml     | 17 +++++++++++++++++
 4 files changed, 26 insertions(+), 11 deletions(-)
 create mode 100644 releasenotes/notes/bug-2074219-937d6404c1cbb04c.yaml

diff --git a/nova/tests/functional/regressions/test_bug_2074219.py b/nova/tests/functional/regressions/test_bug_2074219.py
index cbc672ed1e..36ccfbbb9e 100644
--- a/nova/tests/functional/regressions/test_bug_2074219.py
+++ b/nova/tests/functional/regressions/test_bug_2074219.py
@@ -12,7 +12,6 @@
 # under the License.
 
 from nova.tests.fixtures import libvirt as fakelibvirt
-from nova.tests.functional.api import client
 from nova.tests.functional.libvirt import test_vgpu
 
 
@@ -61,13 +60,7 @@ class VGPUTestVolumeOPs(test_vgpu.VGPUTestBase):
         # Detach the volume from the instance
         # DELETE /servers/{server_id}/os-volume_attachments/{volume_id} is
         # async but as we are using CastAsCall it's sync in our func tests
-        # FIXME(sean-k-mooney): This test is currently broken because of
-        # bug 2074219
-        # self.api.delete_server_volume(
-        #     server_id, self.cinder.IMAGE_BACKED_VOL)
-        # self._wait_for_volume_detach(
-        #     server_id, self.cinder.IMAGE_BACKED_VOL)
-        error = self.assertRaises(
-            client.OpenStackApiException, self.api.delete_server_volume,
+        self.api.delete_server_volume(
+            server_id, self.cinder.IMAGE_BACKED_VOL)
+        self._wait_for_volume_detach(
             server_id, self.cinder.IMAGE_BACKED_VOL)
-        self.assertEqual(500, error.response.status_code)
diff --git a/nova/tests/unit/virt/libvirt/test_guest.py b/nova/tests/unit/virt/libvirt/test_guest.py
index 49ba7c87ae..6d9eb6ede5 100644
--- a/nova/tests/unit/virt/libvirt/test_guest.py
+++ b/nova/tests/unit/virt/libvirt/test_guest.py
@@ -308,6 +308,11 @@ class GuestTestCase(test.NoDBTestCase):
         <address domain='0x0000' bus='0x06' slot='0x12' function='0x6'/>
       </source>
     </hostdev>
+    <hostdev mode='subsystem' type='mdev' model='vfio-pci'>
+      <source>
+        <address uuid='c2177883-f1bb-47f0-914d-32a22e3a8804'/>
+      </source>
+    </hostdev>
     <interface type="bridge">
       <mac address="fa:16:3e:f9:af:ae"/>
       <model type="virtio"/>
diff --git a/nova/virt/libvirt/guest.py b/nova/virt/libvirt/guest.py
index c8d9611587..44bb7daf7e 100644
--- a/nova/virt/libvirt/guest.py
+++ b/nova/virt/libvirt/guest.py
@@ -416,7 +416,7 @@ class Guest(object):
     def get_device_by_alias(self, devalias, devtype=None,
                             from_persistent_config=False):
         for dev in self.get_all_devices(devtype):
-            if dev.alias == devalias:
+            if hasattr(dev, 'alias') and dev.alias == devalias:
                 return dev
 
     def get_all_devices(
diff --git a/releasenotes/notes/bug-2074219-937d6404c1cbb04c.yaml b/releasenotes/notes/bug-2074219-937d6404c1cbb04c.yaml
new file mode 100644
index 0000000000..d823fb9bad
--- /dev/null
+++ b/releasenotes/notes/bug-2074219-937d6404c1cbb04c.yaml
@@ -0,0 +1,17 @@
+---
+fixes:
+  - |
+    During the Caracal cycle the libvirt driver was enhanced to support using
+    device aliases to detach devices from a domain.
+    I1dfe4ad3df81bc810835af9b09cfc6c06e9a5388
+    This introduced a regression for instance with vgpus.
+    A prior bugfix https://bugs.launchpad.net/nova/+bug/1942345
+    addressed the symptom without correcting the underlying problem.
+    A related bug for mdev devices was later reported.
+    https://bugs.launchpad.net/nova/+bug/2074219
+    When this feature was added nova introduced a helper method
+    to get device via the alias because the libvirt api does not provide one
+    natively. That helper function assumed all devices would have an alias
+    attribute. That assumption was not valid and had now been corrected.
+    As a result detaching a volume from an instance with vgpus should now
+    be possible and this class of bug should no longer happen.