libvirt: Check the guest support UEFI

commit 539d381434 add a
NVRAM flag in guest delete_configuration, but check
UEFI support only in host side. If an instance does
not boot from UEFI, but the host enable UEFI software
like OVMF, the NVRAM flag always delete.

This patch also check the guest support UEFI.

Change-Id: I3448b89f58f3c3a85d15ee5a870c24b4b7f201df
Closes-Bug: #1845628

nova/tests/unit/virt/libvirt/test_driver.py

@@ -15822,8 +15822,11 @@ class LibvirtConnTestCase(test.NoDBTestCase,
         mock_domain.undefine.assert_called_once_with()
         mock_save.assert_called_once_with()
 
+    @mock.patch('nova.utils.get_image_from_system_metadata')
     @mock.patch.object(objects.Instance, 'save')
-    def test_destroy_removes_nvram(self, mock_save):
+    def test_destroy_removes_nvram_host_support_uefi(self,
+                                                     mock_save,
+                                                     mock_image):
         mock_domain = mock.Mock(fakelibvirt.virDomain)
         mock_domain.ID.return_value = 123
 
@@ -15835,15 +15838,47 @@ class LibvirtConnTestCase(test.NoDBTestCase,
             state=power_state.SHUTDOWN, internal_id=-1))
 
         instance = objects.Instance(self.context, **self.test_instance)
+        mock_image.return_value = {"properties": {
+            "hw_firmware_type": "bios"}}
         drvr.destroy(self.context, instance, [])
 
         self.assertEqual(1, mock_domain.ID.call_count)
         mock_domain.destroy.assert_called_once_with()
-        # undefineFlags should now be called with 5 as uefi us supported
+        # NVRAM flag should not called only host support uefi
         mock_domain.undefineFlags.assert_called_once_with(
+            fakelibvirt.VIR_DOMAIN_UNDEFINE_MANAGED_SAVE
+        )
+        mock_domain.undefine.assert_not_called()
+        mock_save.assert_called_once_with()
+
+    @mock.patch('nova.utils.get_image_from_system_metadata')
+    @mock.patch.object(objects.Instance, 'save')
+    def test_destroy_removes_nvram_host_and_guest_support_uefi(self,
+                                                               mock_save,
+                                                               mock_image):
+        mock_domain = mock.Mock(fakelibvirt.virDomain)
+        mock_domain.ID.return_value = 123
+
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
+        drvr._host._get_domain = mock.Mock(return_value=mock_domain)
+        drvr._has_uefi_support = mock.Mock(return_value=True)
+        drvr.delete_instance_files = mock.Mock(return_value=None)
+        drvr.get_info = mock.Mock(return_value=hardware.InstanceInfo(
+            state=power_state.SHUTDOWN, internal_id=-1))
+
+        instance = objects.Instance(self.context, **self.test_instance)
+        mock_image.return_value = {"properties": {
+            "hw_firmware_type": "uefi"}}
+        drvr.destroy(self.context, instance, [])
+
+        self.assertEqual(1, mock_domain.ID.call_count)
+        mock_domain.destroy.assert_called_once_with()
+        # undefineFlags should now be called with 5 as uefi supported
+        # by both host and guest
+        mock_domain.undefineFlags.assert_has_calls([mock.call(
             fakelibvirt.VIR_DOMAIN_UNDEFINE_MANAGED_SAVE |
             fakelibvirt.VIR_DOMAIN_UNDEFINE_NVRAM
-        )
+        )])
         mock_domain.undefine.assert_not_called()
         mock_save.assert_called_once_with()
 
@@ -18730,6 +18765,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
     def _test_swap_volume(self, mock_is_job_complete, source_type,
                           resize=False, fail=False):
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI())
+        image_meta = objects.ImageMeta.from_dict(self.test_image_meta)
+        hw_firmware_type = image_meta.properties.get(
+            'hw_firmware_type')
 
         mock_dom = mock.MagicMock()
         guest = libvirt_guest.Guest(mock_dom)
@@ -18769,10 +18807,11 @@ class LibvirtConnTestCase(test.NoDBTestCase,
             mock_conf = mock.MagicMock(source_type=source_type,
                                        source_path=dstfile)
             if not fail:
-                drvr._swap_volume(guest, srcfile, mock_conf, 1)
+                drvr._swap_volume(guest, srcfile, mock_conf, 1,
+                                  hw_firmware_type)
             else:
                 self.assertRaises(expected_exception, drvr._swap_volume, guest,
-                                  srcfile, mock_conf, 1)
+                                  srcfile, mock_conf, 1, hw_firmware_type)
 
             # Verify we read the original persistent config.
             expected_call_count = 1
@@ -18829,6 +18868,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                          swap_volume, disconnect_volume):
         conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI())
         instance = objects.Instance(**self.test_instance)
+        image_meta = objects.ImageMeta.from_dict(self.test_image_meta)
+        hw_firmware_type = image_meta.properties.get(
+            'hw_firmware_type')
         old_connection_info = {'driver_volume_type': 'fake',
                                'serial': 'old-volume-id',
                                'data': {'device_path': '/fake-old-volume',
@@ -18861,7 +18903,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
         connect_volume.assert_called_once_with(self.context,
                                                new_connection_info, instance)
 
-        swap_volume.assert_called_once_with(guest, 'vdb', conf, 1)
+        swap_volume.assert_called_once_with(guest, 'vdb',
+                                            conf, 1, hw_firmware_type)
         disconnect_volume.assert_called_once_with(self.context,
                                                   old_connection_info,
                                                   instance)

nova/virt/libvirt/driver.py

@@ -1314,7 +1314,10 @@ class LibvirtDriver(driver.ComputeDriver):
         try:
             guest = self._host.get_guest(instance)
             try:
-                support_uefi = self._has_uefi_support()
+                hw_firmware_type = instance.image_meta.properties.get(
+                    'hw_firmware_type')
+                support_uefi = (self._has_uefi_support() and
+                                hw_firmware_type == fields.FirmwareType.UEFI)
                 guest.delete_configuration(support_uefi)
             except libvirt.libvirtError as e:
                 with excutils.save_and_reraise_exception() as ctxt:
@@ -1842,7 +1845,8 @@ class LibvirtDriver(driver.ComputeDriver):
                 self._disconnect_volume(context, connection_info, instance,
                                         encryption=encryption)
 
-    def _swap_volume(self, guest, disk_path, conf, resize_to):
+    def _swap_volume(self, guest, disk_path,
+                     conf, resize_to, hw_firmware_type):
         """Swap existing disk with a new block device."""
         dev = guest.get_block_device(disk_path)
 
@@ -1863,7 +1867,8 @@ class LibvirtDriver(driver.ComputeDriver):
             #             If any part of this block fails, the domain is
             #             re-defined regardless.
             if guest.has_persistent_configuration():
-                support_uefi = self._has_uefi_support()
+                support_uefi = (self._has_uefi_support() and
+                                hw_firmware_type == fields.FirmwareType.UEFI)
                 guest.delete_configuration(support_uefi)
 
             try:
@@ -1933,8 +1938,12 @@ class LibvirtDriver(driver.ComputeDriver):
             self._disconnect_volume(context, new_connection_info, instance)
             raise NotImplementedError(_("Swap only supports host devices"))
 
+        hw_firmware_type = instance.image_meta.properties.get(
+            'hw_firmware_type')
+
         try:
-            self._swap_volume(guest, disk_dev, conf, resize_to)
+            self._swap_volume(guest, disk_dev, conf,
+                              resize_to, hw_firmware_type)
         except exception.VolumeRebaseFailed:
             with excutils.save_and_reraise_exception():
                 self._disconnect_volume(context, new_connection_info, instance)
@@ -2538,7 +2547,10 @@ class LibvirtDriver(driver.ComputeDriver):
             #             If any part of this block fails, the domain is
             #             re-defined regardless.
             if guest.has_persistent_configuration():
-                support_uefi = self._has_uefi_support()
+                hw_firmware_type = image_meta.properties.get(
+                    'hw_firmware_type')
+                support_uefi = (self._has_uefi_support() and
+                                hw_firmware_type == fields.FirmwareType.UEFI)
                 guest.delete_configuration(support_uefi)
 
             # NOTE (rmk): Establish a temporary mirror of our root disk and

releasenotes/notes/bug-1845628-3152e73a1e4856b2.yaml

@@ -0,0 +1,13 @@
+---
+fixes:
+  - |
+    Prior to this release Nova determined if ``UEFI`` support should be
+    enable solely by checking host support as reported in `bug 1845628`_.
+
+    Nova now correctly check for guest os support via the ``hw_firmware_type``
+    image metadata property when spawning new instance and only
+    enables ``UEFI`` if the guest and host support it.
+    Guest deletion has also been updated to correctly clean up based on
+    the ``UEFI`` or ``BIOS`` configuration of the vm.
+
+    .. _bug 1845628: https://bugs.launchpad.net/nova/+bug/1845628