From cef63d267cab5c08f1e1d17af8ea139835a8fd6d Mon Sep 17 00:00:00 2001 From: Antoni Segura Puimedon Date: Thu, 26 Apr 2018 15:28:08 +0200 Subject: [PATCH] Fix octavia lb deleting_race with cascading Octavia can cascade resource deletion by passing &cascade=True as a parameter to the HTTP request. This patch enables us to do that, fixing the children deletion race and greatly simplifying the callstack of service deletion Closes-Bug: #1767074 Change-Id: I97303b54b900bc40f791309bd2589e617b61177a Signed-off-by: Antoni Segura Puimedon --- kuryr_kubernetes/clients.py | 2 ++ .../controller/drivers/lbaasv2.py | 32 +++++++++++++------ kuryr_kubernetes/controller/handlers/lbaas.py | 14 ++++++-- .../unit/controller/drivers/test_lbaasv2.py | 18 +++++++++++ .../unit/controller/handlers/test_lbaas.py | 25 +++++++++++++++ 5 files changed, 79 insertions(+), 12 deletions(-) diff --git a/kuryr_kubernetes/clients.py b/kuryr_kubernetes/clients.py index ca9006ecd..b00350ce9 100644 --- a/kuryr_kubernetes/clients.py +++ b/kuryr_kubernetes/clients.py @@ -52,6 +52,7 @@ def setup_loadbalancer_client(): if any(ext['alias'] == 'lbaasv2' for ext in neutron_client.list_extensions()['extensions']): _clients[_LB_CLIENT] = neutron_client + neutron_client.cascading_capable = False else: # Since Octavia is lbaasv2 API compatible (A superset of it) we'll just # wire an extra neutron client instance to point to it @@ -63,6 +64,7 @@ def setup_loadbalancer_client(): service_type='load-balancer') lbaas_client.httpclient = octo_httpclient _clients[_LB_CLIENT] = lbaas_client + lbaas_client.cascading_capable = True def setup_kubernetes_client(): diff --git a/kuryr_kubernetes/controller/drivers/lbaasv2.py b/kuryr_kubernetes/controller/drivers/lbaasv2.py index b4891e640..4725b1f3f 100644 --- a/kuryr_kubernetes/controller/drivers/lbaasv2.py +++ b/kuryr_kubernetes/controller/drivers/lbaasv2.py @@ -40,6 +40,11 @@ _SUPPORTED_LISTENER_PROT = ('HTTP', 'HTTPS', 'TCP') class LBaaSv2Driver(base.LBaaSDriver): """LBaaSv2Driver implements LBaaSDriver for Neutron LBaaSv2 API.""" + @property + def cascading_capable(self): + lbaas = clients.get_loadbalancer_client() + return lbaas.cascading_capable + def ensure_loadbalancer(self, endpoints, project_id, subnet_id, ip, security_groups_ids, service_type): name = "%(namespace)s/%(name)s" % endpoints['metadata'] @@ -67,16 +72,25 @@ class LBaaSv2Driver(base.LBaaSDriver): def release_loadbalancer(self, endpoints, loadbalancer): neutron = clients.get_neutron_client() lbaas = clients.get_loadbalancer_client() - self._release(loadbalancer, loadbalancer, - lbaas.delete_loadbalancer, loadbalancer.id) + if lbaas.cascading_capable: + self._release( + loadbalancer, + loadbalancer, + lbaas.delete, + lbaas.lbaas_loadbalancer_path % loadbalancer.id, + params={'cascade': True}) - sg_id = self._find_listeners_sg(loadbalancer) - if sg_id: - try: - neutron.delete_security_group(sg_id) - except n_exc.NeutronClientException: - LOG.exception('Error when deleting loadbalancer security ' - 'group. Leaving it orphaned.') + else: + self._release(loadbalancer, loadbalancer, + lbaas.delete_loadbalancer, loadbalancer.id) + + sg_id = self._find_listeners_sg(loadbalancer) + if sg_id: + try: + neutron.delete_security_group(sg_id) + except n_exc.NeutronClientException: + LOG.exception('Error when deleting loadbalancer security ' + 'group. Leaving it orphaned.') def ensure_security_groups(self, endpoints, loadbalancer, security_groups_ids, service_type): diff --git a/kuryr_kubernetes/controller/handlers/lbaas.py b/kuryr_kubernetes/controller/handlers/lbaas.py index c78d804ed..defa58839 100644 --- a/kuryr_kubernetes/controller/handlers/lbaas.py +++ b/kuryr_kubernetes/controller/handlers/lbaas.py @@ -269,9 +269,17 @@ class LoadBalancerHandler(k8s_base.ResourceEventHandler): if not lbaas_state: return # NOTE(ivc): deleting pool deletes its members - lbaas_state.members = [] - self._sync_lbaas_members(endpoints, lbaas_state, - obj_lbaas.LBaaSServiceSpec()) + if self._drv_lbaas.cascading_capable: + self._drv_lbaas.release_loadbalancer( + endpoints=endpoints, + loadbalancer=lbaas_state.loadbalancer) + if lbaas_state.service_pub_ip_info: + self._drv_service_pub_ip.release_pub_ip( + lbaas_state.service_pub_ip_info) + else: + lbaas_state.members = [] + self._sync_lbaas_members(endpoints, lbaas_state, + obj_lbaas.LBaaSServiceSpec()) def _should_ignore(self, endpoints, lbaas_spec): return not(lbaas_spec and diff --git a/kuryr_kubernetes/tests/unit/controller/drivers/test_lbaasv2.py b/kuryr_kubernetes/tests/unit/controller/drivers/test_lbaasv2.py index c9a966ede..0a457d93c 100644 --- a/kuryr_kubernetes/tests/unit/controller/drivers/test_lbaasv2.py +++ b/kuryr_kubernetes/tests/unit/controller/drivers/test_lbaasv2.py @@ -75,6 +75,7 @@ class TestLBaaSv2Driver(test_base.TestCase): def test_release_loadbalancer(self): self.useFixture(k_fix.MockNeutronClient()).client lbaas = self.useFixture(k_fix.MockLBaaSClient()).client + lbaas.cascading_capable = False cls = d_lbaasv2.LBaaSv2Driver m_driver = mock.Mock(spec=d_lbaasv2.LBaaSv2Driver) endpoints = mock.sentinel.endpoints @@ -86,6 +87,23 @@ class TestLBaaSv2Driver(test_base.TestCase): lbaas.delete_loadbalancer, loadbalancer.id) + def test_cascade_release_loadbalancer(self): + self.useFixture(k_fix.MockNeutronClient()).client + lbaas = self.useFixture(k_fix.MockLBaaSClient()).client + lbaas.cascading_capable = True + lbaas.lbaas_loadbalancer_path = "boo %s" + cls = d_lbaasv2.LBaaSv2Driver + m_driver = mock.Mock(spec=d_lbaasv2.LBaaSv2Driver) + endpoints = mock.sentinel.endpoints + loadbalancer = mock.Mock() + + cls.release_loadbalancer(m_driver, endpoints, loadbalancer) + + m_driver._release.assert_called_once_with( + loadbalancer, loadbalancer, lbaas.delete, + lbaas.lbaas_loadbalancer_path % loadbalancer.id, + params={'cascade': True}) + def test_ensure_listener(self): cls = d_lbaasv2.LBaaSv2Driver m_driver = mock.Mock(spec=d_lbaasv2.LBaaSv2Driver) diff --git a/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py b/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py index fff28b2e0..e07ba60f5 100644 --- a/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py +++ b/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py @@ -516,6 +516,8 @@ class TestLoadBalancerHandler(test_base.TestCase): m_handler = mock.Mock(spec=h_lbaas.LoadBalancerHandler) m_handler._get_lbaas_state.return_value = lbaas_state + m_handler._drv_lbaas = mock.Mock() + m_handler._drv_lbaas.cascading_capable = False h_lbaas.LoadBalancerHandler.on_deleted(m_handler, endpoints) @@ -523,6 +525,29 @@ class TestLoadBalancerHandler(test_base.TestCase): m_handler._sync_lbaas_members.assert_called_once_with( endpoints, lbaas_state, empty_spec) + @mock.patch('kuryr_kubernetes.objects.lbaas' + '.LBaaSServiceSpec') + def test_on_cascade_deleted_lb_service(self, m_svc_spec_ctor): + endpoints = mock.sentinel.endpoints + empty_spec = mock.sentinel.empty_spec + lbaas_state = mock.Mock() + lbaas_state.loadbalancer = mock.sentinel.loadbalancer + lbaas_state.service_pub_ip_info = mock.sentinel.pub_ip + m_svc_spec_ctor.return_value = empty_spec + + m_handler = mock.Mock(spec=h_lbaas.LoadBalancerHandler) + m_handler._get_lbaas_state.return_value = lbaas_state + m_handler._drv_lbaas = mock.Mock() + m_handler._drv_service_pub_ip = mock.Mock() + m_handler._drv_lbaas.cascading_capable = True + + h_lbaas.LoadBalancerHandler.on_deleted(m_handler, endpoints) + + m_handler._drv_lbaas.release_loadbalancer.assert_called_once_with( + endpoints=endpoints, loadbalancer=lbaas_state.loadbalancer) + m_handler._drv_service_pub_ip.release_pub_ip.assert_called_once_with( + lbaas_state.service_pub_ip_info) + def test_should_ignore(self): endpoints = mock.sentinel.endpoints lbaas_spec = mock.sentinel.lbaas_spec