neutron: Unbind remaining ports after PortNotFound

Just because we encountered a PortNotFound error when unbinding a port
doesn't mean we should stop unbinding the remaining ports. If this error
is encountered, simply continue with the other ports.

While we're here, we clean up some other tests related to '_unbind_port'
since they're clearly duplicates.

Change-Id: Id04e0df12829df4d8929e03a8b76b5cbe0549059
Signed-off-by: Stephen Finucane <[email protected]>
Closes-Bug: #1974173

Author: stephenfin

nova/network/neutron.py

@@ -636,6 +636,7 @@ def _unbind_ports(self, context, ports,
             # in case the caller forgot to filter the list.
             if port_id is None:
                 continue
+
             port_req_body: ty.Dict[str, ty.Any] = {
                 'port': {
                     'device_id': '',
@@ -650,7 +651,7 @@ def _unbind_ports(self, context, ports,
             except exception.PortNotFound:
                 LOG.debug('Unable to show port %s as it no longer '
                           'exists.', port_id)
-                return
+                continue
             except Exception:
                 # NOTE: In case we can't retrieve the binding:profile or
                 # network info assume that they are empty

nova/tests/unit/network/test_neutron.py

@@ -5248,7 +5248,8 @@ def test_get_preexisting_port_ids(self, mock_get_nw_info):
         self.assertEqual(['2', '3'], result, "Invalid preexisting ports")
 
     @mock.patch('nova.network.neutron.API._show_port')
-    def _test_unbind_ports_get_client(self, mock_neutron, mock_show):
+    @mock.patch('nova.network.neutron.get_client')
+    def test_unbind_ports_get_client(self, mock_neutron, mock_show):
         mock_ctx = mock.Mock(is_admin=False)
         ports = ["1", "2", "3"]
 
@@ -5264,25 +5265,18 @@ def _test_unbind_ports_get_client(self, mock_neutron, mock_show):
         self.assertEqual(1, mock_neutron.call_count)
         mock_neutron.assert_has_calls(get_client_calls, True)
 
-    @mock.patch('nova.network.neutron.get_client')
-    def test_unbind_ports_get_client_binding_extension(self,
-                                                       mock_neutron):
-        self._test_unbind_ports_get_client(mock_neutron)
-
-    @mock.patch('nova.network.neutron.get_client')
-    def test_unbind_ports_get_client(self, mock_neutron):
-        self._test_unbind_ports_get_client(mock_neutron)
-
     @mock.patch('nova.network.neutron.API.has_dns_extension',
                 new=mock.Mock(return_value=False))
     @mock.patch('nova.network.neutron.API._show_port')
-    def _test_unbind_ports(self, mock_neutron, mock_show):
+    @mock.patch('nova.network.neutron.get_client')
+    def test_unbind_ports(self, mock_neutron, mock_show):
         mock_client = mock.Mock()
         mock_update_port = mock.Mock()
         mock_client.update_port = mock_update_port
         mock_ctx = mock.Mock(is_admin=False)
         ports = ["1", "2", "3"]
         mock_show.side_effect = [{"id": "1"}, {"id": "2"}, {"id": "3"}]
+
         api = neutronapi.API()
         api._unbind_ports(mock_ctx, ports, mock_neutron, mock_client)
 
@@ -5296,14 +5290,6 @@ def _test_unbind_ports(self, mock_neutron, mock_show):
         self.assertEqual(3, mock_update_port.call_count)
         mock_update_port.assert_has_calls(update_port_calls)
 
-    @mock.patch('nova.network.neutron.get_client')
-    def test_unbind_ports_binding_ext(self, mock_neutron):
-        self._test_unbind_ports(mock_neutron)
-
-    @mock.patch('nova.network.neutron.get_client')
-    def test_unbind_ports(self, mock_neutron):
-        self._test_unbind_ports(mock_neutron)
-
     def test_unbind_ports_no_port_ids(self):
         # Tests that None entries in the ports list are filtered out.
         mock_client = mock.Mock()
@@ -6068,7 +6054,6 @@ def test_get_instance_id_by_floating_address_port_not_found(self,
     def test_unbind_ports_port_show_portnotfound(self, mock_log, mock_show):
         api = neutronapi.API()
         neutron_client = mock.Mock()
-        mock_show.return_value = {'id': uuids.port}
         api._unbind_ports(self.context, [uuids.port_id],
                           neutron_client, neutron_client)
         mock_show.assert_called_once_with(
@@ -6077,6 +6062,63 @@ def test_unbind_ports_port_show_portnotfound(self, mock_log, mock_show):
             neutron_client=mock.ANY)
         mock_log.assert_not_called()
 
+    @mock.patch(
+        'nova.network.neutron.API.has_dns_extension',
+        new=mock.Mock(return_value=False),
+    )
+    @mock.patch('nova.network.neutron.API._show_port')
+    @mock.patch.object(neutronapi, 'LOG')
+    def test_unbind_ports_port_show_portnotfound_multiple_ports(
+        self, mock_log, mock_show,
+    ):
+        """Ensure we continue unbinding ports even when one isn't found."""
+        mock_show.side_effect = [
+            exception.PortNotFound(port_id=uuids.port_a),
+            {'id': uuids.port_b},
+        ]
+        api = neutronapi.API()
+        neutron_client = mock.Mock()
+
+        api._unbind_ports(
+            self.context,
+            [uuids.port_a, uuids.port_b],
+            neutron_client,
+            neutron_client,
+        )
+
+        mock_show.assert_has_calls(
+            [
+                mock.call(
+                    self.context,
+                    uuids.port_a,
+                    fields=['binding:profile', 'network_id'],
+                    neutron_client=neutron_client,
+                ),
+                mock.call(
+                    self.context,
+                    uuids.port_b,
+                    fields=['binding:profile', 'network_id'],
+                    neutron_client=neutron_client,
+                ),
+            ]
+        )
+        # Only the port that exists should be updated
+        neutron_client.update_port.assert_called_once_with(
+            uuids.port_b,
+            {
+                'port': {
+                    'device_id': '',
+                    'device_owner': '',
+                    'binding:profile': {},
+                    'binding:host_id': None,
+                }
+            }
+        )
+        mock_log.exception.assert_not_called()
+        mock_log.debug.assert_called_with(
+            'Unable to show port %s as it no longer exists.', uuids.port_a,
+        )
+
     @mock.patch('nova.network.neutron.API.has_dns_extension',
                 new=mock.Mock(return_value=False))
     @mock.patch('nova.network.neutron.API._show_port',
@@ -6100,7 +6142,7 @@ def test_unbind_ports_port_show_unexpected_error(self,
                 new=mock.Mock(return_value=False))
     @mock.patch('nova.network.neutron.API._show_port')
     @mock.patch.object(neutronapi.LOG, 'exception')
-    def test_unbind_ports_portnotfound(self, mock_log, mock_show):
+    def test_unbind_ports_port_update_portnotfound(self, mock_log, mock_show):
         api = neutronapi.API()
         neutron_client = mock.Mock()
         neutron_client.update_port = mock.Mock(
@@ -6118,7 +6160,9 @@ def test_unbind_ports_portnotfound(self, mock_log, mock_show):
                 new=mock.Mock(return_value=False))
     @mock.patch('nova.network.neutron.API._show_port')
     @mock.patch.object(neutronapi.LOG, 'exception')
-    def test_unbind_ports_unexpected_error(self, mock_log, mock_show):
+    def test_unbind_ports_port_update_unexpected_error(
+        self, mock_log, mock_show,
+    ):
         api = neutronapi.API()
         neutron_client = mock.Mock()
         neutron_client.update_port = mock.Mock(