Merge "Add a lock to prevent race during detach/attach of interface"

Author: Zuul

nova/compute/manager.py

@@ -7504,6 +7504,19 @@ def _deallocate_port_for_instance(self, context, instance, port_id,
     def attach_interface(self, context, instance, network_id, port_id,
                          requested_ip, tag):
         """Use hotplug to add an network adapter to an instance."""
+        lockname = 'interface-%s-%s' % (instance.uuid, port_id)
+
+        @utils.synchronized(lockname)
+        def do_attach_interface(context, instance, network_id, port_id,
+                                requested_ip, tag):
+            return self._attach_interface(context, instance, network_id,
+                                port_id, requested_ip, tag)
+
+        return do_attach_interface(context, instance, network_id, port_id,
+                                   requested_ip, tag)
+
+    def _attach_interface(self, context, instance, network_id, port_id,
+                          requested_ip, tag):
         if not self.driver.capabilities.get('supports_attach_interface',
                                             False):
             raise exception.AttachInterfaceNotSupported(
@@ -7563,6 +7576,18 @@ def attach_interface(self, context, instance, network_id, port_id,
     @wrap_instance_fault
     def detach_interface(self, context, instance, port_id):
         """Detach a network adapter from an instance."""
+        lockname = 'interface-%s-%s' % (instance.uuid, port_id)
+
+        @utils.synchronized(lockname)
+        def do_detach_interface(context, instance, port_id):
+            self._detach_interface(context, instance, port_id)
+
+        do_detach_interface(context, instance, port_id)
+
+    def _detach_interface(self, context, instance, port_id):
+        # NOTE(aarents): we need to refresh info cache from DB here,
+        # as previous detach/attach lock holder just updated it.
+        compute_utils.refresh_info_cache_for_instance(context, instance)
         network_info = instance.info_cache.network_info
         condemned = None
         for vif in network_info:

nova/tests/unit/compute/test_compute.py

@@ -10284,11 +10284,15 @@ def test_attach_interface(self, mock_notify):
         network_id = nwinfo[0]['network']['id']
         port_id = nwinfo[0]['id']
         req_ip = '1.2.3.4'
+        lock_name = 'interface-%s-%s' % (instance.uuid, port_id)
         mock_allocate = mock.Mock(return_value=nwinfo)
         self.compute.network_api.allocate_port_for_instance = mock_allocate
 
-        with mock.patch.dict(self.compute.driver.capabilities,
-                             supports_attach_interface=True):
+        with test.nested(
+            mock.patch.dict(self.compute.driver.capabilities,
+                             supports_attach_interface=True),
+            mock.patch('oslo_concurrency.lockutils.lock')
+        ) as (cap, mock_lock):
             vif = self.compute.attach_interface(self.context,
                                                 instance,
                                                 network_id,
@@ -10303,6 +10307,9 @@ def test_attach_interface(self, mock_notify):
                       action='interface_attach', phase='start'),
             mock.call(self.context, instance, self.compute.host,
                       action='interface_attach', phase='end')])
+        mock_lock.assert_called_once_with(lock_name, mock.ANY, mock.ANY,
+                mock.ANY, delay=mock.ANY, do_log=mock.ANY, fair=mock.ANY,
+                semaphores=mock.ANY)
         return nwinfo, port_id
 
     @mock.patch.object(compute_utils, 'notify_about_instance_action')
@@ -10400,6 +10407,7 @@ def test_detach_interface(self, mock_notify):
             self.context, uuids.info_cache_instance)
         instance.info_cache.network_info = network_model.NetworkInfo.hydrate(
             nwinfo)
+        lock_name = 'interface-%s-%s' % (instance.uuid, port_id)
 
         port_allocation = {uuids.rp1: {'NET_BW_EGR_KILOBIT_PER_SEC': 10000}}
         with test.nested(
@@ -10408,8 +10416,9 @@ def test_detach_interface(self, mock_notify):
                     'remove_resources_from_instance_allocation'),
                 mock.patch.object(self.compute.network_api,
                     'deallocate_port_for_instance',
-                    return_value=([], port_allocation))) as (
-                mock_remove_alloc, mock_deallocate):
+                    return_value=([], port_allocation)),
+                mock.patch('oslo_concurrency.lockutils.lock')) as (
+                mock_remove_alloc, mock_deallocate, mock_lock):
             self.compute.detach_interface(self.context, instance, port_id)
 
             mock_deallocate.assert_called_once_with(
@@ -10423,6 +10432,9 @@ def test_detach_interface(self, mock_notify):
                       action='interface_detach', phase='start'),
             mock.call(self.context, instance, self.compute.host,
                       action='interface_detach', phase='end')])
+        mock_lock.assert_called_once_with(lock_name, mock.ANY, mock.ANY,
+                mock.ANY, delay=mock.ANY, do_log=mock.ANY, fair=mock.ANY,
+                semaphores=mock.ANY)
 
     @mock.patch('nova.compute.manager.LOG.log')
     def test_detach_interface_failed(self, mock_log):

nova/tests/unit/compute/test_compute_mgr.py

@@ -2586,8 +2586,9 @@ def test_detach_interface_failure(self):
 
         @mock.patch.object(compute_utils, 'EventReporter')
         @mock.patch.object(compute_utils, 'add_instance_fault_from_exc')
+        @mock.patch.object(compute_utils, 'refresh_info_cache_for_instance')
         @mock.patch.object(self.compute, '_set_instance_obj_error_state')
-        def do_test(meth, add_fault, event):
+        def do_test(meth, refresh, add_fault, event):
             self.assertRaises(exception.PortNotFound,
                               self.compute.detach_interface,
                               self.context, f_instance, 'port_id')
@@ -2596,15 +2597,17 @@ def do_test(meth, add_fault, event):
             event.assert_called_once_with(
                 self.context, 'compute_detach_interface', CONF.host,
                 f_instance.uuid, graceful_exit=False)
+            refresh.assert_called_once_with(self.context, f_instance)
 
         do_test()
 
     @mock.patch('nova.compute.manager.LOG.log')
     @mock.patch.object(compute_utils, 'EventReporter')
     @mock.patch.object(compute_utils, 'add_instance_fault_from_exc')
     @mock.patch.object(compute_utils, 'notify_about_instance_action')
-    def test_detach_interface_instance_not_found(self, mock_notify, mock_fault,
-                                                 mock_event, mock_log):
+    @mock.patch.object(compute_utils, 'refresh_info_cache_for_instance')
+    def test_detach_interface_instance_not_found(self, mock_refresh,
+                            mock_notify, mock_fault, mock_event, mock_log):
         nw_info = network_model.NetworkInfo([
             network_model.VIF(uuids.port_id)])
         info_cache = objects.InstanceInfoCache(network_info=nw_info,
@@ -2617,6 +2620,7 @@ def test_detach_interface_instance_not_found(self, mock_notify, mock_fault,
             self.assertRaises(exception.InterfaceDetachFailed,
                               self.compute.detach_interface,
                               self.context, instance, uuids.port_id)
+            mock_refresh.assert_called_once_with(self.context, instance)
             self.assertEqual(1, mock_log.call_count)
             self.assertEqual(logging.DEBUG, mock_log.call_args[0][0])
 

releasenotes/notes/bug-1892870-eb894956bf04713d.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Resolve a race condition that may occur during concurrent
+    ``interface detach/attach``, resulting in an interface accidentally unbind
+    after attached. See `bug 1892870`_ for more details.
+
+    .. _bug 1892870: https://bugs.launchpad.net/nova/+bug/1892870