Fix unplugging member ports

Unplugging a port from a VM can take some time before the update is
committed to the DB, 2 (quick) successive member deletions can trigger a
race condition when computing the delta of the ports that need to be
removed/added: Octavia may get a list of ports connected to an amphora
that is not correct.

This commit ensures that a port is also removed from the DB when
unplugging a member port.

Story 2009864
Task 44543

Change-Id: Iaa8ea5255b225ef87461c1f85b80f36210b1bde4

octavia/network/drivers/neutron/allowed_address_pairs.py

@@ -619,9 +619,26 @@ class AllowedAddressPairsDriver(neutron_base.BaseNeutronDriver):
 
         unpluggers = self._get_interfaces_to_unplug(interfaces, network_id,
                                                     ip_address=ip_address)
+        removed_port_ids = set()
         for index, unplugger in enumerate(unpluggers):
             self.compute.detach_port(
                 compute_id=compute_id, port_id=unplugger.port_id)
+            removed_port_ids.add(unplugger.port_id)
+
+        port_detach_timeout = CONF.networking.port_detach_timeout
+
+        start = time.time()
+        while time.time() - start < port_detach_timeout:
+            interfaces = self.get_plugged_networks(compute_id)
+            plugged_port_ids = {i.port_id for i in interfaces}
+            if not plugged_port_ids & removed_port_ids:
+                break
+            time.sleep(CONF.networking.retry_interval)
+        else:
+            LOG.warning("Ports (%s) still attached to compute %s after "
+                        "%s seconds.",
+                        ", ".join(removed_port_ids),
+                        compute_id, port_detach_timeout)
 
     def update_vip(self, load_balancer, for_delete=False):
         sec_grp = self._get_lb_security_group(load_balancer.id)

octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py

@@ -798,7 +798,10 @@ class TestAllowedAddressPairsDriver(base.TestCase):
         self.assertEqual(t_constants.MOCK_PORT_ID, vip.port_id)
         self.assertEqual(fake_lb.id, vip.load_balancer_id)
 
-    def test_unplug_aap_port_errors_when_update_port_cant_find_port(self):
+    @mock.patch("time.time")
+    @mock.patch("time.sleep")
+    def test_unplug_aap_port_errors_when_update_port_cant_find_port(
+            self, mock_time_sleep, mock_time_time):
         lb = dmh.generate_load_balancer_tree()
         list_ports = self.driver.neutron_client.list_ports
         port1 = t_constants.MOCK_NEUTRON_PORT['port']
@@ -809,14 +812,22 @@ class TestAllowedAddressPairsDriver(base.TestCase):
         subnet = network_models.Subnet(
             id=t_constants.MOCK_MANAGEMENT_SUBNET_ID,
             network_id='3')
-        list_ports.return_value = {'ports': [port1, port2]}
+        list_ports.side_effect = [
+            {'ports': [port1, port2]},
+            {'ports': [port1, port2]},
+            {'ports': [port1]}
+        ]
+        mock_time_time.side_effect = [1, 1, 2]
         update_port = self.driver.neutron_client.update_port
         update_port.side_effect = neutron_exceptions.PortNotFoundClient
         self.assertRaises(network_base.UnplugVIPException,
                           self.driver.unplug_aap_port, lb.vip, lb.amphorae[0],
                           subnet)
 
-    def test_unplug_aap_errors_when_update_port_fails(self):
+    @mock.patch("time.time")
+    @mock.patch("time.sleep")
+    def test_unplug_aap_errors_when_update_port_fails(
+            self, mock_time_sleep, mock_time_time):
         lb = dmh.generate_load_balancer_tree()
         port1 = t_constants.MOCK_NEUTRON_PORT['port']
         port2 = {
@@ -829,7 +840,12 @@ class TestAllowedAddressPairsDriver(base.TestCase):
             network_id='3')
 
         list_ports = self.driver.neutron_client.list_ports
-        list_ports.return_value = {'ports': [port1, port2]}
+        list_ports.side_effect = [
+            {'ports': [port1, port2]},
+            {'ports': [port1, port2]},
+            {'ports': [port1]}
+        ]
+        mock_time_time.side_effect = [1, 1, 2]
 
         update_port = self.driver.neutron_client.update_port
         update_port.side_effect = TypeError
@@ -853,7 +869,9 @@ class TestAllowedAddressPairsDriver(base.TestCase):
         self.driver.unplug_vip(lb, lb.vip)
         self.assertEqual(len(lb.amphorae), mock.call_count)
 
-    def test_unplug_aap_port(self):
+    @mock.patch("time.time")
+    @mock.patch("time.sleep")
+    def test_unplug_aap_port(self, mock_time_sleep, mock_time_time):
         lb = dmh.generate_load_balancer_tree()
         update_port = self.driver.neutron_client.update_port
         port1 = t_constants.MOCK_NEUTRON_PORT['port']
@@ -865,7 +883,12 @@ class TestAllowedAddressPairsDriver(base.TestCase):
             id=t_constants.MOCK_MANAGEMENT_SUBNET_ID,
             network_id='3')
         list_ports = self.driver.neutron_client.list_ports
-        list_ports.return_value = {'ports': [port1, port2]}
+        list_ports.side_effect = [
+            {'ports': [port1, port2]},
+            {'ports': [port1, port2]},
+            {'ports': [port1]}
+        ]
+        mock_time_time.side_effect = [1, 1, 2]
         get_port = self.driver.neutron_client.get_port
         get_port.side_effect = neutron_exceptions.NotFound
         self.driver.unplug_aap_port(lb.vip, lb.amphorae[0], subnet)
@@ -929,7 +952,36 @@ class TestAllowedAddressPairsDriver(base.TestCase):
                           self.driver.unplug_network,
                           t_constants.MOCK_COMPUTE_ID, net_id)
 
-    def test_unplug_network(self):
+    @mock.patch("time.time")
+    @mock.patch("time.sleep")
+    def test_unplug_network(self, mock_time_sleep, mock_time_time):
+        list_ports = self.driver.neutron_client.list_ports
+        port1 = t_constants.MOCK_NEUTRON_PORT['port']
+        port2 = {
+            'id': '4', 'network_id': '3', 'fixed_ips':
+            [{'ip_address': '10.0.0.2'}]
+        }
+        list_ports.side_effect = [
+            {'ports': [port1, port2]},
+            {'ports': [port1, port2]},
+            {'ports': [port1]}
+        ]
+        port_detach = self.driver.compute.detach_port
+
+        mock_time_time.side_effect = [1, 1, 2]
+
+        self.driver.unplug_network(t_constants.MOCK_COMPUTE_ID,
+                                   port2.get('network_id'))
+        port_detach.assert_called_once_with(
+            compute_id=t_constants.MOCK_COMPUTE_ID, port_id=port2.get('id'))
+
+        mock_time_sleep.assert_called_once()
+
+    @mock.patch("time.time")
+    @mock.patch("time.sleep")
+    @mock.patch("octavia.network.drivers.neutron.allowed_address_pairs.LOG")
+    def test_unplug_network_timeout(self, mock_log,
+                                    mock_time_sleep, mock_time_time):
         list_ports = self.driver.neutron_client.list_ports
         port1 = t_constants.MOCK_NEUTRON_PORT['port']
         port2 = {
@@ -938,11 +990,17 @@ class TestAllowedAddressPairsDriver(base.TestCase):
         }
         list_ports.return_value = {'ports': [port1, port2]}
         port_detach = self.driver.compute.detach_port
+
+        mock_time_time.side_effect = [0, 0, 1, 2, 10, 20, 100, 300]
+
         self.driver.unplug_network(t_constants.MOCK_COMPUTE_ID,
                                    port2.get('network_id'))
         port_detach.assert_called_once_with(
             compute_id=t_constants.MOCK_COMPUTE_ID, port_id=port2.get('id'))
 
+        self.assertEqual(6, len(mock_time_sleep.mock_calls))
+        mock_log.warning.assert_called_once()
+
     def test_update_vip(self):
         lc_1 = data_models.ListenerCidr('l1', '10.0.101.0/24')
         lc_2 = data_models.ListenerCidr('l2', '10.0.102.0/24')

releasenotes/notes/fix-unplugging-member-ports-262b35426e570edd.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Fix a bug that could have triggered a race condition when configuring a
+    member interface in the amphora. Due to a race condition, a network
+    interface might have been deleted from the amphora, leading to a loss of
+    connectivity.