Merge "Unbind port when offloading a shelved instance"

6 files changed, 73 insertions, 26 deletions

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index efcdece81a..5ea71827fc 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -6874,6 +6874,9 @@ class ComputeManager(manager.Manager):
         current_power_state = self._get_power_state(instance)
         network_info = self.network_api.get_instance_nw_info(context, instance)
 
+        ports_id = [vif['id'] for vif in network_info]
+        self.network_api.unbind_ports(context, ports_id, detach=False)
+
         block_device_info = self._get_instance_block_device_info(context,
                                                                  instance,
                                                                  bdms=bdms)
diff --git a/nova/network/neutron.py b/nova/network/neutron.py
index 27e7d06455..6c9f19f010 100644
--- a/nova/network/neutron.py
+++ b/nova/network/neutron.py
@@ -612,10 +612,22 @@ class API:
                     raise exception.ExternalNetworkAttachForbidden(
                         network_uuid=net['id'])
 
+    def unbind_ports(self, context, ports, detach=True):
+        """Unbind and detach the given ports by clearing their
+        device_owner and dns_name.
+        The device_id will also be cleaned if detach=True.
+
+        :param context: The request context.
+        :param ports: list of port IDs.
+        """
+        neutron = get_client(context)
+        self._unbind_ports(context, ports, neutron, detach=detach)
+
     def _unbind_ports(self, context, ports,
-                      neutron, port_client=None):
-        """Unbind the given ports by clearing their device_id,
+                      neutron, port_client=None, detach=True):
+        """Unbind and detach the given ports by clearing their
         device_owner and dns_name.
+        The device_id will also be cleaned if detach=True.
 
         :param context: The request context.
         :param ports: list of port IDs.
@@ -638,11 +650,12 @@ class API:
 
             port_req_body: ty.Dict[str, ty.Any] = {
                 'port': {
-                    'device_id': '',
-                    'device_owner': '',
                     constants.BINDING_HOST_ID: None,
                 }
             }
+            if detach:
+                port_req_body['port']['device_id'] = ''
+                port_req_body['port']['device_owner'] = ''
             try:
                 port = self._show_port(
                     context, port_id, neutron_client=neutron,
diff --git a/nova/tests/functional/libvirt/test_pci_sriov_servers.py b/nova/tests/functional/libvirt/test_pci_sriov_servers.py
index 6b8b254af9..135a457154 100644
--- a/nova/tests/functional/libvirt/test_pci_sriov_servers.py
+++ b/nova/tests/functional/libvirt/test_pci_sriov_servers.py
@@ -1612,10 +1612,7 @@ class VDPAServersTest(_PCIServersWithMigrationTestBase):
         # to any host but should still be owned by the vm
         port = self.neutron.show_port(vdpa_port['id'])['port']
         self.assertEqual(server['id'], port['device_id'])
-        # FIXME(sean-k-mooney): we should be unbinding the port from
-        # the host when we shelve offload but we don't today.
-        # This is unrelated to vdpa port and is a general issue.
-        self.assertEqual(hostname, port['binding:host_id'])
+        self.assertIsNone(port['binding:host_id'])
         self.assertIn('binding:profile', port)
         self.assertIsNone(server['OS-EXT-SRV-ATTR:hypervisor_hostname'])
         self.assertIsNone(server['OS-EXT-SRV-ATTR:host'])
@@ -1637,9 +1634,7 @@ class VDPAServersTest(_PCIServersWithMigrationTestBase):
         self.assertPCIDeviceCounts(hostname, total=num_pci, free=num_pci)
         self.assertIsNone(server['OS-EXT-SRV-ATTR:hypervisor_hostname'])
         port = self.neutron.show_port(vdpa_port['id'])['port']
-        # FIXME(sean-k-mooney): shelve  offload should unbind the port
-        # self.assertEqual('', port['binding:host_id'])
-        self.assertEqual(hostname, port['binding:host_id'])
+        self.assertIsNone(port['binding:host_id'])
 
         server = self._unshelve_server(server)
         self.assertPCIDeviceCounts(hostname, total=num_pci, free=num_pci - 2)
@@ -1670,9 +1665,7 @@ class VDPAServersTest(_PCIServersWithMigrationTestBase):
         self.assertPCIDeviceCounts(source, total=num_pci, free=num_pci)
         self.assertIsNone(server['OS-EXT-SRV-ATTR:hypervisor_hostname'])
         port = self.neutron.show_port(vdpa_port['id'])['port']
-        # FIXME(sean-k-mooney): shelve should unbind the port
-        # self.assertEqual('', port['binding:host_id'])
-        self.assertEqual(source, port['binding:host_id'])
+        self.assertIsNone(port['binding:host_id'])
 
         # force the unshelve to the other host
         self.api.put_service(
@@ -3926,17 +3919,7 @@ class RemoteManagedServersTest(_PCIServersWithMigrationTestBase):
 
         port = self.neutron.show_port(uuids.dpu_tunnel_port)['port']
         self.assertIn('binding:profile', port)
-        self.assertEqual(
-            {
-                'pci_vendor_info': '15b3:101e',
-                'pci_slot': '0000:82:00.4',
-                'physical_network': None,
-                'pf_mac_address': '52:54:00:1e:59:02',
-                'vf_num': 3,
-                'card_serial_number': 'MT0000X00002',
-            },
-            port['binding:profile'],
-        )
+        self.assertEqual({}, port['binding:profile'])
 
     def test_suspend(self):
         self.start_compute()
diff --git a/nova/tests/functional/test_servers.py b/nova/tests/functional/test_servers.py
index 43208aa812..5887c99081 100644
--- a/nova/tests/functional/test_servers.py
+++ b/nova/tests/functional/test_servers.py
@@ -6526,3 +6526,41 @@ class PortAndFlavorAccelsServerCreateTest(AcceleratorServerBase):
         binding_profile = neutronapi.get_binding_profile(updated_port)
         self.assertNotIn('arq_uuid', binding_profile)
         self.assertNotIn('pci_slot', binding_profile)
+
+
+class PortBindingShelvedServerTest(integrated_helpers._IntegratedTestBase):
+    """Tests for servers with ports."""
+
+    compute_driver = 'fake.SmallFakeDriver'
+
+    def setUp(self):
+        super(PortBindingShelvedServerTest, self).setUp()
+        self.flavor_id = self._create_flavor(
+            disk=10, ephemeral=20, swap=5 * 1024)
+
+    def test_shelve_offload_with_port(self):
+        # Do not wait before offloading
+        self.flags(shelved_offload_time=0)
+
+        server = self._create_server(
+            flavor_id=self.flavor_id,
+            networks=[{'port': self.neutron.port_1['id']}])
+
+        port = self.neutron.show_port(self.neutron.port_1['id'])['port']
+
+        # Assert that the port is actually associated to the instance
+        self.assertEqual(port['device_id'], server['id'])
+        self.assertEqual(port['binding:host_id'], 'compute')
+        self.assertEqual(port['binding:status'], 'ACTIVE')
+
+        # Do shelve
+        server = self._shelve_server(server, 'SHELVED_OFFLOADED')
+
+        # Retrieve the updated port
+        port = self.neutron.show_port(self.neutron.port_1['id'])['port']
+
+        # Assert that the port is still associated to the instance
+        # but the binding is not on the compute anymore
+        self.assertEqual(port['device_id'], server['id'])
+        self.assertIsNone(port['binding:host_id'])
+        self.assertNotIn('binding:status', port)
diff --git a/nova/tests/unit/compute/test_shelve.py b/nova/tests/unit/compute/test_shelve.py
index f95a722ced..0a1e3f54fc 100644
--- a/nova/tests/unit/compute/test_shelve.py
+++ b/nova/tests/unit/compute/test_shelve.py
@@ -209,6 +209,7 @@ class ShelveComputeManagerTestCase(test_compute.BaseTestCase):
         instance = self._shelve_offload(clean_shutdown=False)
         mock_power_off.assert_called_once_with(instance, 0, 0)
 
+    @mock.patch.object(neutron_api.API, 'unbind_ports')
     @mock.patch.object(compute_utils, 'EventReporter')
     @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_instance_uuid')
     @mock.patch.object(nova.compute.manager.ComputeManager,
@@ -225,7 +226,7 @@ class ShelveComputeManagerTestCase(test_compute.BaseTestCase):
     def _shelve_offload(self, mock_notify, mock_notify_instance_usage,
                         mock_get_power_state, mock_update_resource_tracker,
                         mock_delete_alloc, mock_terminate, mock_get_bdms,
-                        mock_event, clean_shutdown=True):
+                        mock_event, mock_unbind_ports, clean_shutdown=True):
         host = 'fake-mini'
         instance = self._create_fake_instance_obj(params={'host': host})
         instance.task_state = task_states.SHELVING
@@ -278,6 +279,9 @@ class ShelveComputeManagerTestCase(test_compute.BaseTestCase):
                                            instance.uuid,
                                            graceful_exit=False)
 
+        mock_unbind_ports.assert_called_once_with(
+            self.context, mock.ANY, detach=False)
+
         return instance
 
     @mock.patch('nova.compute.utils.'
diff --git a/releasenotes/notes/port-binding-removed-shelved-offloaded-f1772a64be007b24.yaml b/releasenotes/notes/port-binding-removed-shelved-offloaded-f1772a64be007b24.yaml
new file mode 100644
index 0000000000..7e2dccbbf4
--- /dev/null
+++ b/releasenotes/notes/port-binding-removed-shelved-offloaded-f1772a64be007b24.yaml
@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    [`bug 1983471 <https://bugs.launchpad.net/nova/+bug/1983471>`_]
+    When offloading a shelved instance, the compute will now remove the
+    binding so instance ports will appear as "unbound" in neutron.