From 887ffeaefdc5ee3b3d164cd80533bfa190a3704b Mon Sep 17 00:00:00 2001 From: Gregory Thiemonge Date: Thu, 17 Feb 2022 17:29:25 +0100 Subject: [PATCH] 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 (cherry picked from commit 17a79b7ebf1b6e2faf8266b66a46ea83411359f3) --- .../drivers/neutron/allowed_address_pairs.py | 17 +++++ .../neutron/test_allowed_address_pairs.py | 72 +++++++++++++++++-- ...lugging-member-ports-262b35426e570edd.yaml | 7 ++ 3 files changed, 89 insertions(+), 7 deletions(-) create mode 100644 releasenotes/notes/fix-unplugging-member-ports-262b35426e570edd.yaml diff --git a/octavia/network/drivers/neutron/allowed_address_pairs.py b/octavia/network/drivers/neutron/allowed_address_pairs.py index e7fa1c501f..bb783742b9 100644 --- a/octavia/network/drivers/neutron/allowed_address_pairs.py +++ b/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) diff --git a/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py b/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py index 710ce6028a..0d4bdd8ab4 100644 --- a/octavia/tests/unit/network/drivers/neutron/test_allowed_address_pairs.py +++ b/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') diff --git a/releasenotes/notes/fix-unplugging-member-ports-262b35426e570edd.yaml b/releasenotes/notes/fix-unplugging-member-ports-262b35426e570edd.yaml new file mode 100644 index 0000000000..f0937c03c4 --- /dev/null +++ b/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.